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] Fix query fetchPolicy and deduplication #73199

Merged

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Jul 24, 2020

Summary

This PR introduces queryDeduplication param to the useWithSource to make it possible to apolloClient to differentiate parallel requests from each other and make sure that the cached response from one request was not returned in a different place

Checklist

@patrykkopycinski patrykkopycinski added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 24, 2020
@spong
Copy link
Member

spong commented Jul 28, 2020

Checked out, tested locally, and paired with @patrykkopycinski over the changes as well. Was able to confirm the following three fixes:

  • No more disappearing Alerts Table or failure to load data when the Alerts Histogram is successful
  • Top-N hover action no longer sometimes fails to load and is displayed as expected
  • Raw events are still displayed as fixed in [SECURITY] [Timeline] Raw events not displayed #72387 (no regression! 🙌 )

Did a bit of smoke testing and didn't see any fallout from these changes other than the below two items. The extra cancelled query is 🆗 it seems, but if possible we should see if we can keep the Alerts Table in its default loading state as part of this PR (might be an easy fix as seems to only happen on hard refresh, so cache + render state fix?).

@andrew-goldstein & @peluja1012 -- since this changes back to a cache-first policy in some areas if you could desk test this as well the extra 👀 would be appreciated. The queryDeduplication fix for the hooks combined with the cache policy changes seems to be 👍 so far.

Extra cancelled query within Timeline

This should be fine, but just wanted to note the extra cancelled query when within Timeline. This can be seen when switching between event types.

Alerts Table empty container loading state

@patrykkopycinski patrykkopycinski marked this pull request as ready for review July 28, 2020 12:07
@patrykkopycinski patrykkopycinski requested review from a team as code owners July 28, 2020 12:07
@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
securitySolution 7.3MB +443.0B 7.3MB

History

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

@@ -433,7 +434,7 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
closeAddExceptionModal,
]);

if (loading || isEmpty(signalsIndex)) {
if (loading || indexPatternsLoading || isEmpty(signalsIndex)) {
Copy link
Member

Choose a reason for hiding this comment

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

💙 🎉

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out the latest and the Alerts Table loading looks good now! Thanks @patrykkopycinski!

This LGTM, but let's wait on one more 👍 for added testing before merging.

@spong spong added Feature:Detection Rules Security Solution rules and Detection Engine Team:SIEM v7.10.0 labels Jul 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @patrykkopycinski 🙏
Desk tested locally
LGTM 🚀

Copy link
Contributor

@peluja1012 peluja1012 left a comment

Choose a reason for hiding this comment

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

Pulled down branch and verified that the Exception modal bugs and the empty alerts table bug are no longer there. Looks good! Thank you.

@patrykkopycinski patrykkopycinski added bug Fixes for quality problems that affect the customer experience and removed v7.10.0 labels Jul 28, 2020
@patrykkopycinski patrykkopycinski changed the title Fix query [Security Solution] Fix query fetchPolicy and deduplication Jul 28, 2020
@patrykkopycinski patrykkopycinski merged commit 5e62450 into elastic:master Jul 28, 2020
@patrykkopycinski patrykkopycinski deleted the fix/use-with-source-cache branch July 28, 2020 20:13
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Jul 28, 2020
spong added a commit that referenced this pull request Jul 28, 2020
… issues (#73233)

## Summary

Fixes the following issues around Risk Score/Severity mapping:
* Severity override option cannot be unselected during rule creation
* Risk score override option cannot be unselected during rule creation
* Cannot fill Critical Severity override at the first attempt
* Cannot create a rule with just a Critical severity override

Note: When editing rules there is the possibility of the mapping fields remaining `disabled` as they are locked to the 'isLoading' flag from the gql `useFetchIndexPatterns` call, which can sometimes not return/get stuck as loading. @patrykkopycinski has a draft PR to fix this here: #73199

cc @MadameSheema 


##### Severity Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497829-b653de00-cf7e-11ea-8e14-c351117b4282.gif" />
</p>


Now distinguishes between empty string/value
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497776-94f2f200-cf7e-11ea-821e-3766b7bed3dc.png" />
</p>

##### Risk Score Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497842-c075dc80-cf7e-11ea-8c41-606b20a6ac1c.gif" />
</p>


### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [X] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
  * Working with @benskelker on API docs. This PR adds `risk_score` (can be `undefined`) to `risk_score.mapping` for future compatibility with mapping to specific risk score values.
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit to spong/kibana that referenced this pull request Jul 28, 2020
… issues (elastic#73233)

## Summary

Fixes the following issues around Risk Score/Severity mapping:
* Severity override option cannot be unselected during rule creation
* Risk score override option cannot be unselected during rule creation
* Cannot fill Critical Severity override at the first attempt
* Cannot create a rule with just a Critical severity override

Note: When editing rules there is the possibility of the mapping fields remaining `disabled` as they are locked to the 'isLoading' flag from the gql `useFetchIndexPatterns` call, which can sometimes not return/get stuck as loading. @patrykkopycinski has a draft PR to fix this here: elastic#73199

cc @MadameSheema 


##### Severity Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497829-b653de00-cf7e-11ea-8e14-c351117b4282.gif" />
</p>


Now distinguishes between empty string/value
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497776-94f2f200-cf7e-11ea-821e-3766b7bed3dc.png" />
</p>

##### Risk Score Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497842-c075dc80-cf7e-11ea-8c41-606b20a6ac1c.gif" />
</p>


### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [X] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
  * Working with @benskelker on API docs. This PR adds `risk_score` (can be `undefined`) to `risk_score.mapping` for future compatibility with mapping to specific risk score values.
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit to spong/kibana that referenced this pull request Jul 28, 2020
… issues (elastic#73233)

## Summary

Fixes the following issues around Risk Score/Severity mapping:
* Severity override option cannot be unselected during rule creation
* Risk score override option cannot be unselected during rule creation
* Cannot fill Critical Severity override at the first attempt
* Cannot create a rule with just a Critical severity override

Note: When editing rules there is the possibility of the mapping fields remaining `disabled` as they are locked to the 'isLoading' flag from the gql `useFetchIndexPatterns` call, which can sometimes not return/get stuck as loading. @patrykkopycinski has a draft PR to fix this here: elastic#73199

cc @MadameSheema 


##### Severity Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497829-b653de00-cf7e-11ea-8e14-c351117b4282.gif" />
</p>


Now distinguishes between empty string/value
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497776-94f2f200-cf7e-11ea-821e-3766b7bed3dc.png" />
</p>

##### Risk Score Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497842-c075dc80-cf7e-11ea-8c41-606b20a6ac1c.gif" />
</p>


### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [X] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
  * Working with @benskelker on API docs. This PR adds `risk_score` (can be `undefined`) to `risk_score.mapping` for future compatibility with mapping to specific risk score values.
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit that referenced this pull request Jul 28, 2020
… issues (#73233) (#73543)

## Summary

Fixes the following issues around Risk Score/Severity mapping:
* Severity override option cannot be unselected during rule creation
* Risk score override option cannot be unselected during rule creation
* Cannot fill Critical Severity override at the first attempt
* Cannot create a rule with just a Critical severity override

Note: When editing rules there is the possibility of the mapping fields remaining `disabled` as they are locked to the 'isLoading' flag from the gql `useFetchIndexPatterns` call, which can sometimes not return/get stuck as loading. @patrykkopycinski has a draft PR to fix this here: #73199

cc @MadameSheema 


##### Severity Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497829-b653de00-cf7e-11ea-8e14-c351117b4282.gif" />
</p>


Now distinguishes between empty string/value
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497776-94f2f200-cf7e-11ea-821e-3766b7bed3dc.png" />
</p>

##### Risk Score Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497842-c075dc80-cf7e-11ea-8c41-606b20a6ac1c.gif" />
</p>


### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [X] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
  * Working with @benskelker on API docs. This PR adds `risk_score` (can be `undefined`) to `risk_score.mapping` for future compatibility with mapping to specific risk score values.
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit that referenced this pull request Jul 29, 2020
… issues (#73233) (#73542)

## Summary

Fixes the following issues around Risk Score/Severity mapping:
* Severity override option cannot be unselected during rule creation
* Risk score override option cannot be unselected during rule creation
* Cannot fill Critical Severity override at the first attempt
* Cannot create a rule with just a Critical severity override

Note: When editing rules there is the possibility of the mapping fields remaining `disabled` as they are locked to the 'isLoading' flag from the gql `useFetchIndexPatterns` call, which can sometimes not return/get stuck as loading. @patrykkopycinski has a draft PR to fix this here: #73199

cc @MadameSheema 


##### Severity Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497829-b653de00-cf7e-11ea-8e14-c351117b4282.gif" />
</p>


Now distinguishes between empty string/value
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497776-94f2f200-cf7e-11ea-821e-3766b7bed3dc.png" />
</p>

##### Risk Score Mapping Fixes:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/88497842-c075dc80-cf7e-11ea-8c41-606b20a6ac1c.gif" />
</p>


### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [X] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
  * Working with @benskelker on API docs. This PR adds `risk_score` (can be `undefined`) to `risk_score.mapping` for future compatibility with mapping to specific risk score values.
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

Co-authored-by: Elastic Machine <[email protected]>
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Security Solution rules and Detection Engine release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants