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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Failed getFieldsForIndexPattern calls can result in Exception Flyout getting stuck in loading state #158371

Merged
merged 7 commits into from May 30, 2023
Merged

[Security Solution] Failed getFieldsForIndexPattern calls can result in Exception Flyout getting stuck in loading state #158371

merged 7 commits into from May 30, 2023

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented May 24, 2023 •
edited by kibanamachine

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. 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 :-)

@e40pud
[Security Solution] Failed getFieldsForIndexPattern calls can result …
bd3b79d 
…in Exception Flyout getting stuck in loading state (elastic#158110)
@e40pud e40pud added release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. ci:cloud-deploy Create or update a Cloud deployment Team:Detection Engine Security Solution Detection Engine Area labels May 24, 2023
@e40pud e40pud self-assigned this May 24, 2023
@e40pud e40pud marked this pull request as ready for review May 24, 2023 12:21
@e40pud e40pud requested review from a team as code owners May 24, 2023 12:21
@elasticmachine
Copy link
Contributor

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

@e40pud
Copy link
Contributor Author

e40pud commented May 24, 2023 •
edited

Closing this PR in favour of #155900
I cherry-picked my changes there, so that we avoid conflicts during merging.

cc @yctercero @dhurley14

@e40pud e40pud closed this May 24, 2023
@e40pud e40pud mentioned this pull request May 24, 2023
Closed
@dhurley14
Copy link
Contributor

These changes are smaller than my PR and we want to get this fix in for the next patch release so I'm re-opening this PR

@dhurley14 dhurley14 reopened this May 26, 2023
@e40pud
Copy link
Contributor Author

e40pud commented May 27, 2023

@elasticmachine merge upstream

@e40pud e40pud requested a review from a team as a code owner May 30, 2023 10:34
@e40pud
Copy link
Contributor Author

e40pud commented May 30, 2023

@elasticmachine merge upstream

e40pud
e40pud commented May 30, 2023
View reviewed changes
...ecurity_solution/public/detection_engine/rule_exceptions/logic/use_exception_flyout_data.tsx Outdated
pattern: '',
includeUnmapped: true,
});
const fields = dv.fields.map((field) => field.toSpec());
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 duplication of the code above x-pack/plugins/security_solution/public/common/containers/source/index.tsx

It is refactored in #155900. Left duplication to avoid unnecessary conflicts when we gonna merge that PR, since conflict description code is copied from there.

dhurley14
dhurley14 reviewed May 30, 2023
View reviewed changes
x-pack/plugins/security_solution/public/common/containers/source/index.tsx Outdated
Comment on lines 157 to 176
const fieldNameConflictDescriptionsMap = (await data.dataViews
.getFieldsForIndexPattern(dv, {
pattern: '',
includeUnmapped: true,
})
.then((fieldsToReduce) => {
return fieldsToReduce.reduce(
(acc, f) => ({ ...acc, [f.name]: f.conflictDescriptions }),
{} as { [x: string]: Record<string, string[]> | undefined }
);
})) as { [x: string]: Record<string, string[]> | undefined };

dv.fields.replaceAll([
...fields.map((field) => ({
...field,
...(fieldNameConflictDescriptionsMap[field.name] != null
? { conflictDescriptions: fieldNameConflictDescriptionsMap[field.name] }
: {}),
})),
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my poor-performing code - sorry I offered it up as a working solution from my other PR.

Anyways, I feel we can reduce this to a single iteration with the below code and it should work:

Suggested change
const fieldNameConflictDescriptionsMap = (await data.dataViews
.getFieldsForIndexPattern(dv, {
pattern: '',
includeUnmapped: true,
})
.then((fieldsToReduce) => {
return fieldsToReduce.reduce(
(acc, f) => ({ ...acc, [f.name]: f.conflictDescriptions }),
{} as { [x: string]: Record<string, string[]> | undefined }
);
})) as { [x: string]: Record<string, string[]> | undefined };
dv.fields.replaceAll([
...fields.map((field) => ({
...field,
...(fieldNameConflictDescriptionsMap[field.name] != null
? { conflictDescriptions: fieldNameConflictDescriptionsMap[field.name] }
: {}),
})),
]);
const fieldNameConflictDescriptionsMap =
await data.dataViews.getFieldsForIndexPattern(dv, {
pattern: '',
includeUnmapped: true,
});
fieldNameConflictDescriptionsMap.forEach((field) => {
dv.fields.update({ ...field });
});

@e40pud
Copy link
Contributor Author

e40pud commented May 30, 2023

@elasticmachine merge upstream

@e40pud e40pud requested a review from dhurley14 May 30, 2023 18:32
@kibana-ci
Copy link
Collaborator

kibana-ci commented May 30, 2023 •
edited

💚 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
securitySolution 9.2MB 9.2MB +278.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 416 420 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 500 504 +4
total +6

History

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

cc @e40pud

@dhurley14 dhurley14 merged commit 1a3cad1 into elastic:main May 30, 2023
20 checks passed
@kibanamachine kibanamachine mentioned this pull request May 30, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 30, 2023
@e40pud
[Security Solution] Failed getFieldsForIndexPattern calls can result …
ae2a153 
…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
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 31, 2023
@kibanamachine @e40pud
[8.8] [Security Solution] Failed getFieldsForIndexPattern calls can r…
caa8e8e 
…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>
@e40pud e40pud mentioned this pull request Jun 6, 2023
Closed
@yctercero yctercero mentioned this pull request Jun 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhurley14 dhurley14 dhurley14 approved these changes

@yctercero yctercero Awaiting requested review from yctercero

Assignees

@e40pud e40pud

Labels
ci:cloud-deploy Create or update a Cloud deployment release_note:fix Team:Detection Engine Security Solution Detection Engine Area Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.8.1 v8.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@e40pud @elasticmachine @dhurley14 @kibana-ci @kibanamachine