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

[Security Solution][Alerts] Provide more information about rule exception behavior before creation #149149

Merged

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Jan 18, 2023

Summary

These changes surface mapping issues when exceptions are created. We gonna warn the user about type conflicts and unmapped indices.

Tooltip warning inside the field selection dropdown menu:

Screenshot 2023-01-18 at 19 01 44

Warning text underneath the dropdown menu when user picks the field which has mapping issues:

Screen.Recording.2023-01-30.at.12.23.12.mov

Main ticket #146845

@e40pud e40pud added release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Solution Platform Security Solution Platform Team Team:Detection Alerts Security Detection Alerts Area Team ci:cloud-deploy Create or update a Cloud deployment labels Jan 18, 2023
@e40pud e40pud requested a review from a team as a code owner January 18, 2023 18:20
@e40pud e40pud self-assigned this Jan 18, 2023
@e40pud e40pud requested review from a team as code owners January 18, 2023 18:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

/**
* Description of field type conflicts across indices
*/
conflictDescriptions?: Record<string, string[]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this remain unchanged. I'm kind of curious how fields are being loaded where this would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted these changes. Instead I extended DataViewFieldBase here in the kbn-securitysolution-list-utils package where we use conflict descriptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@e40pud It still might be worth having a conversation about how the field list is being loaded to make sure the data views api is doing what it can in this circumstance.

Copy link
Contributor

Choose a reason for hiding this comment

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

@elastic/security-solution-platform - @e40pud and I had a conversation about how fields are being loaded and it seems like the data views api could be doing more when dealing with unmapped fields. My goal is that you should be able to get the fields you need and related data in a single call. Please reach out to me if you'd like to discuss this further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for a reference, here is the place where I do extra call to get unmapped and conflicts information in data views case.

@e40pud e40pud requested a review from a team as a code owner January 19, 2023 10:49
@e40pud e40pud requested a review from mattkime January 19, 2023 12:35
@e40pud
Copy link
Contributor Author

e40pud commented Jan 23, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 25, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 26, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 26, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 30, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Feb 6, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Feb 6, 2023

@nkhristinin We did not discuss such a possibility, but I would expect that if there are any other places that might benefit from this we can introduce these changes there as well. Should be easy to enable it elsewhere in Security Solution. I will talk about it with the team.

Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Overall looks good from investigations perspective. Left a small comment.

@@ -158,8 +159,12 @@ export const requestIndexFieldSearch = async (
pattern: index,
});
}
const fieldCapsOptions = request.includeUnmapped
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall looks good.

Do you think this warrants adding tests in x-pack/plugins/timelines/server/search_strategy/index_fields/index.test.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will add tests to cover new changes!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Feb 6, 2023

💚 Build Succeeded

Metrics [docs]

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
@kbn/securitysolution-list-utils 150 155 +5

Async chunks

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

id before after diff
lists 149.3KB 152.1KB +2.7KB
securitySolution 12.9MB 12.9MB +7.7KB
total +10.4KB
Unknown metric groups

API count

id before after diff
@kbn/securitysolution-list-utils 194 202 +8

History

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

cc @e40pud

@e40pud e40pud merged commit 84efdaa into elastic:main Feb 6, 2023
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Feb 6, 2023
darnautov pushed a commit to darnautov/kibana that referenced this pull request Feb 7, 2023
…tion behavior before creation (elastic#149149)

## Summary

These changes surface mapping issues when exceptions are created. We
gonna warn the user about type conflicts and unmapped indices.

Tooltip warning inside the field selection dropdown menu:

<img width="2020" alt="Screenshot 2023-01-18 at 19 01 44"
src="https://user-images.githubusercontent.com/2700761/213261684-61d21068-12bc-408f-8d20-1a196e0719a7.png">

Warning text underneath the dropdown menu when user picks the field
which has mapping issues:


https://user-images.githubusercontent.com/2700761/215467838-5d39ff75-3a2e-44ef-ba89-57cd3975310c.mov

Main ticket elastic#146845

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
benakansara pushed a commit to benakansara/kibana that referenced this pull request Feb 7, 2023
…tion behavior before creation (elastic#149149)

## Summary

These changes surface mapping issues when exceptions are created. We
gonna warn the user about type conflicts and unmapped indices.

Tooltip warning inside the field selection dropdown menu:

<img width="2020" alt="Screenshot 2023-01-18 at 19 01 44"
src="https://user-images.githubusercontent.com/2700761/213261684-61d21068-12bc-408f-8d20-1a196e0719a7.png">

Warning text underneath the dropdown menu when user picks the field
which has mapping issues:


https://user-images.githubusercontent.com/2700761/215467838-5d39ff75-3a2e-44ef-ba89-57cd3975310c.mov

Main ticket elastic#146845

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
e40pud added a commit to e40pud/kibana that referenced this pull request Mar 6, 2023
e40pud added a commit that referenced this pull request Mar 6, 2023
…152726)

## Summary

These changes update warning message that we show to user to indicate
index mapping conflicts while selecting a field to build a Rule
Exception.

New tooltip message:

<img width="829" alt="Screenshot 2023-03-06 at 16 18 51"
src="https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png">

[Main ticket](#146845)
Addition to [this PR](#149149)


cc @nastasha-solomon
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 6, 2023
…lastic#152726)

## Summary

These changes update warning message that we show to user to indicate
index mapping conflicts while selecting a field to build a Rule
Exception.

New tooltip message:

<img width="829" alt="Screenshot 2023-03-06 at 16 18 51"
src="https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png">

[Main ticket](elastic#146845)
Addition to [this PR](elastic#149149)

cc @nastasha-solomon

(cherry picked from commit ce96318)
kibanamachine added a commit that referenced this pull request Mar 6, 2023
…ssage (#152726) (#152755)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution][Alerts] Update mapping conflicts warning message
(#152726)](#152726)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"ievgen.sorokopud@elastic.co"},"sourceCommit":{"committedDate":"2023-03-06T18:23:26Z","message":"[Security
Solution][Alerts] Update mapping conflicts warning message
(#152726)\n\n## Summary\r\n\r\nThese changes update warning message that
we show to user to indicate\r\nindex mapping conflicts while selecting a
field to build a Rule\r\nException.\r\n\r\nNew tooltip
message:\r\n\r\n<img width=\"829\" alt=\"Screenshot 2023-03-06 at 16 18
51\"\r\nsrc=\"https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png\">\r\n\r\n[Main
ticket](#146845 to
[this PR](#149149
@nastasha-solomon","sha":"ce9631850d8631eb72b52687fb5ed0b7645f207d","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:
SecuritySolution","Team:Security Solution Platform","Team:Detection
Alerts","backport:prev-minor","ci:cloud-deploy","v8.8.0"],"number":152726,"url":"#152726
Solution][Alerts] Update mapping conflicts warning message
(#152726)\n\n## Summary\r\n\r\nThese changes update warning message that
we show to user to indicate\r\nindex mapping conflicts while selecting a
field to build a Rule\r\nException.\r\n\r\nNew tooltip
message:\r\n\r\n<img width=\"829\" alt=\"Screenshot 2023-03-06 at 16 18
51\"\r\nsrc=\"https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png\">\r\n\r\n[Main
ticket](#146845 to
[this PR](#149149
@nastasha-solomon","sha":"ce9631850d8631eb72b52687fb5ed0b7645f207d"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"#152726
Solution][Alerts] Update mapping conflicts warning message
(#152726)\n\n## Summary\r\n\r\nThese changes update warning message that
we show to user to indicate\r\nindex mapping conflicts while selecting a
field to build a Rule\r\nException.\r\n\r\nNew tooltip
message:\r\n\r\n<img width=\"829\" alt=\"Screenshot 2023-03-06 at 16 18
51\"\r\nsrc=\"https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png\">\r\n\r\n[Main
ticket](#146845 to
[this PR](#149149
@nastasha-solomon","sha":"ce9631850d8631eb72b52687fb5ed0b7645f207d"}}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <ievgen.sorokopud@elastic.co>
e40pud added a commit that referenced this pull request Mar 7, 2023
…cts functionality (#151366)

## Summary

This PR adds cypress tests to cover new rule exceptions functionality
introduced in [this PR](#149149).

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this pull request Mar 8, 2023
…lastic#152726)

## Summary

These changes update warning message that we show to user to indicate
index mapping conflicts while selecting a field to build a Rule
Exception.

New tooltip message:

<img width="829" alt="Screenshot 2023-03-06 at 16 18 51"
src="https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png">

[Main ticket](elastic#146845)
Addition to [this PR](elastic#149149)


cc @nastasha-solomon
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this pull request Mar 8, 2023
…cts functionality (elastic#151366)

## Summary

This PR adds cypress tests to cover new rule exceptions functionality
introduced in [this PR](elastic#149149).

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
…lastic#152726)

## Summary

These changes update warning message that we show to user to indicate
index mapping conflicts while selecting a field to build a Rule
Exception.

New tooltip message:

<img width="829" alt="Screenshot 2023-03-06 at 16 18 51"
src="https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png">

[Main ticket](elastic#146845)
Addition to [this PR](elastic#149149)


cc @nastasha-solomon
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
…cts functionality (elastic#151366)

## Summary

This PR adds cypress tests to cover new rule exceptions functionality
introduced in [this PR](elastic#149149).

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
…152726)

## Summary

These changes update warning message that we show to user to indicate
index mapping conflicts while selecting a field to build a Rule
Exception.

New tooltip message:

<img width="829" alt="Screenshot 2023-03-06 at 16 18 51"
src="https://user-images.githubusercontent.com/2700761/223154197-ee4ed680-5cc1-4b48-82d8-e225aa24519b.png">

[Main ticket](#146845)
Addition to [this PR](#149149)


cc @nastasha-solomon
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
…cts functionality (#151366)

## Summary

This PR adds cypress tests to cover new rule exceptions functionality
introduced in [this PR](#149149).

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
dhurley14 pushed a commit that referenced this pull request May 30, 2023
…in Exception Flyout getting stuck in loading state (#158371)

## Summary

Original ticket #158110

These changes fixes the issue with the exception flyout which can be
stuck in loading state in case `getFieldsForIndexPattern` throws an
exception.

Fixed by putting the `getFieldsForIndexPattern` call in try/catch. We
use this call to fetch extended information about the fields [to show
warning to the user in case there are some index
issues](#149149). If
`getFieldsForIndexPattern` fails and throws an exception we will
continue using fields without conflicts/unmapped information.

I also, noticed that we do not fetch extended information for the case
where user uses index patterns instead of data views. Fixed this issue
in `useFetchIndex`.

cc @dhurley14 We will need to adjust either of our PRs depending whose
changes will go in first :-)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 30, 2023
…in Exception Flyout getting stuck in loading state (elastic#158371)

## Summary

Original ticket elastic#158110

These changes fixes the issue with the exception flyout which can be
stuck in loading state in case `getFieldsForIndexPattern` throws an
exception.

Fixed by putting the `getFieldsForIndexPattern` call in try/catch. We
use this call to fetch extended information about the fields [to show
warning to the user in case there are some index
issues](elastic#149149). If
`getFieldsForIndexPattern` fails and throws an exception we will
continue using fields without conflicts/unmapped information.

I also, noticed that we do not fetch extended information for the case
where user uses index patterns instead of data views. Fixed this issue
in `useFetchIndex`.

cc @dhurley14 We will need to adjust either of our PRs depending whose
changes will go in first :-)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 1a3cad1)
kibanamachine added a commit that referenced this pull request May 31, 2023
…esult in Exception Flyout getting stuck in loading state (#158371) (#158681)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Security Solution] Failed getFieldsForIndexPattern calls can result
in Exception Flyout getting stuck in loading state
(#158371)](#158371)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"ievgen.sorokopud@elastic.co"},"sourceCommit":{"committedDate":"2023-05-30T20:23:17Z","message":"[Security
Solution] Failed getFieldsForIndexPattern calls can result in Exception
Flyout getting stuck in loading state (#158371)\n\n##
Summary\r\n\r\nOriginal ticket
#158110 changes
fixes the issue with the exception flyout which can be\r\nstuck in
loading state in case `getFieldsForIndexPattern` throws
an\r\nexception.\r\n\r\nFixed by putting the `getFieldsForIndexPattern`
call in try/catch. We\r\nuse this call to fetch extended information
about the fields [to show\r\nwarning to the user in case there are some
index\r\nissues](#149149).
If\r\n`getFieldsForIndexPattern` fails and throws an exception we
will\r\ncontinue using fields without conflicts/unmapped
information.\r\n\r\nI also, noticed that we do not fetch extended
information for the case\r\nwhere user uses index patterns instead of
data views. Fixed this issue\r\nin `useFetchIndex`.\r\n\r\ncc @dhurley14
We will need to adjust either of our PRs depending whose\r\nchanges will
go in first :-)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"1a3cad1b2744a2fb4be070bc5e60754bc183682c","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:
SecuritySolution","ci:cloud-deploy","v8.9.0","v8.8.1","Team:Detection
Engine"],"number":158371,"url":"#158371
Solution] Failed getFieldsForIndexPattern calls can result in Exception
Flyout getting stuck in loading state (#158371)\n\n##
Summary\r\n\r\nOriginal ticket
#158110 changes
fixes the issue with the exception flyout which can be\r\nstuck in
loading state in case `getFieldsForIndexPattern` throws
an\r\nexception.\r\n\r\nFixed by putting the `getFieldsForIndexPattern`
call in try/catch. We\r\nuse this call to fetch extended information
about the fields [to show\r\nwarning to the user in case there are some
index\r\nissues](#149149).
If\r\n`getFieldsForIndexPattern` fails and throws an exception we
will\r\ncontinue using fields without conflicts/unmapped
information.\r\n\r\nI also, noticed that we do not fetch extended
information for the case\r\nwhere user uses index patterns instead of
data views. Fixed this issue\r\nin `useFetchIndex`.\r\n\r\ncc @dhurley14
We will need to adjust either of our PRs depending whose\r\nchanges will
go in first :-)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"1a3cad1b2744a2fb4be070bc5e60754bc183682c"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"#158371
Solution] Failed getFieldsForIndexPattern calls can result in Exception
Flyout getting stuck in loading state (#158371)\n\n##
Summary\r\n\r\nOriginal ticket
#158110 changes
fixes the issue with the exception flyout which can be\r\nstuck in
loading state in case `getFieldsForIndexPattern` throws
an\r\nexception.\r\n\r\nFixed by putting the `getFieldsForIndexPattern`
call in try/catch. We\r\nuse this call to fetch extended information
about the fields [to show\r\nwarning to the user in case there are some
index\r\nissues](#149149).
If\r\n`getFieldsForIndexPattern` fails and throws an exception we
will\r\ncontinue using fields without conflicts/unmapped
information.\r\n\r\nI also, noticed that we do not fetch extended
information for the case\r\nwhere user uses index patterns instead of
data views. Fixed this issue\r\nin `useFetchIndex`.\r\n\r\ncc @dhurley14
We will need to adjust either of our PRs depending whose\r\nchanges will
go in first :-)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"1a3cad1b2744a2fb4be070bc5e60754bc183682c"}},{"branch":"8.8","label":"v8.8.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <ievgen.sorokopud@elastic.co>
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 ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement Team:Detection Alerts Security Detection Alerts Area Team Team:Security Solution Platform Security Solution Platform Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants