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 Solutions][Detection Engine] Removes duplicate API calls #88420

Merged

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Jan 14, 2021

Summary

This removes some duplicate API calls to reduce pressure on the backend and speed up querying times within the application for the front end. This fixes some of the issues of #82327, but there are several performance improvements that are going to be needed to help reduce the slowness when you have a system under a lot of pressure.

So far this removes duplication for these API calls when you are on the manage detection rules page:

api/detection_engine/rules/_find
api/detection_engine/rules/_find_statuses
api/detection_engine/tags

Screen Shot 2021-01-14 at 3 53 21 PM

  • This hides the tags and searches while the page is loading to avoid duplicate calls when the pre-packaged rules counts come back
  • This untangles the refetchRules from the refetchPrePackagedRulesStatus as two separate calls to avoid issues we have with re-rendering and re-calling the backend.

Checklist

@@ -113,9 +113,11 @@ export const useRulesStatuses = (rules: Rules): ReturnRulesStatuses => {
setLoading(false);
}
};
if (rules != null && rules.length > 0) {

if (rules.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unrelated but I notice that sometimes we use Rule[] and sometimes we use Rules. I wonder if sometimes we don't check the type to realize that Rules is just Rule[] and non nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? I just know that we should not check for null if it can never be null or update the types to reflect the runtime reality. In this case it looks like it can never be null.

dispatchRulesInReducer,
}: UseRules): ReturnRules => {
const [rules, setRules] = useState<FetchRulesResponse | null>(null);
const reFetchRules = useRef<(refreshPrePackagedRule?: boolean) => void>(noop);
const [loading, setLoading] = useState(true);
const [, dispatchToaster] = useStateToaster();

const filterTags = filterOptions.tags?.sort().join();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's one more filterOptions.tags? in x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/api.ts that can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it. I updated that one. Thanks for finding it

const found = rules.find((r) => r.id === id);
if (found != null) {
return { [id]: found, ...acc };
}
return acc;
}, {} as Record<string, Rule>);
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 yay for one less as

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't resist when I was in there and just happened by it. Templates are so much better.

setRefreshRulesData(reFetchRulesData);
}
}, [reFetchRulesData, setRefreshRulesData]);
setRefreshRulesData(reFetchRules);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we also update line 93 in this file to reflect the new typing of refetchRules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea. Good catch there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a few more searches and found 4 more uses of that boolean in signatures I'm removing.

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Really love these performance PRs. I pulled down and noticed it felt snippier. Confirmed that there weren't the duplicate calls you'd noted.

There were a few comments that I'm curious to hear what you think.

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Tested locally. 👍

@kibanamachine
Copy link
Contributor

💚 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 8.5MB 8.5MB +467.0B

History

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

@FrankHassanabad FrankHassanabad merged commit 2abbd80 into elastic:master Jan 17, 2021
@FrankHassanabad FrankHassanabad deleted the remove-extra-api-calls branch January 17, 2021 16:18
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Jan 17, 2021
…lastic#88420)

## Summary

This removes some duplicate API calls to reduce pressure on the backend and speed up querying times within the application for the front end. This fixes some of the issues of elastic#82327, but there are several performance improvements that are going to be needed to help reduce the slowness when you have a system under a lot of pressure.

So far this removes duplication for these API calls when you are on the manage detection rules page:

```ts
api/detection_engine/rules/_find
api/detection_engine/rules/_find_statuses
api/detection_engine/tags
```

<img width="2465" alt="Screen Shot 2021-01-14 at 3 53 21 PM" src="https://user-images.githubusercontent.com/1151048/104662295-c031e080-5687-11eb-92d7-18b9ad355646.png">

* This hides the tags and searches while the page is loading to avoid duplicate calls when the pre-packaged rules counts come back
* This untangles the refetchRules from the refetchPrePackagedRulesStatus as two separate calls to avoid issues we have with re-rendering and re-calling the backend.
 
### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
FrankHassanabad added a commit that referenced this pull request Jan 17, 2021
…88420) (#88566)

## Summary

This removes some duplicate API calls to reduce pressure on the backend and speed up querying times within the application for the front end. This fixes some of the issues of #82327, but there are several performance improvements that are going to be needed to help reduce the slowness when you have a system under a lot of pressure.

So far this removes duplication for these API calls when you are on the manage detection rules page:

```ts
api/detection_engine/rules/_find
api/detection_engine/rules/_find_statuses
api/detection_engine/tags
```

<img width="2465" alt="Screen Shot 2021-01-14 at 3 53 21 PM" src="https://user-images.githubusercontent.com/1151048/104662295-c031e080-5687-11eb-92d7-18b9ad355646.png">

* This hides the tags and searches while the page is loading to avoid duplicate calls when the pre-packaged rules counts come back
* This untangles the refetchRules from the refetchPrePackagedRulesStatus as two separate calls to avoid issues we have with re-rendering and re-calling the backend.
 
### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 18, 2021
* master: (33 commits)
  [Security Solution][Case] Fix patch cases integration test with alerts (elastic#88311)
  [Security Solutions][Detection Engine] Removes duplicate API calls (elastic#88420)
  Fix log msg (elastic#88370)
  [Test] Add tag cloud visualization to dashboard in functional test for reporting (elastic#87600)
  removing kibana-core-ui from codeowners (elastic#88111)
  [Alerting] Migrate Event Log plugin to TS project references (elastic#81557)
  [Maps] fix zooming while drawing shape filter logs errors in console (elastic#88413)
  Porting fixes 1 (elastic#88477)
  [APM] Explicitly set environment for cross-service links (elastic#87481)
  chore(NA): remove mocha junit ci integrations (elastic#88129)
  [APM] Only display relevant sections for rum agent in service overview (elastic#88410)
  [Enterprise Search] Automatically mock shared logic files (elastic#88494)
  [APM] Disable Create custom link button on Transaction details page for read-only users
  [Docs] clean-up vega map reference documenation (elastic#88487)
  [Security Solution] Fix Timeline event details layout (elastic#88377)
  Change DELETE to POST for _bulk_delete to avoid incompatibility issues (elastic#87914)
  [Monitoring] Change cloud messaging on no data page (elastic#88375)
  [Uptime] clear ping state when PingList component in unmounted (elastic#88321)
  [APM] Consistent terminology for latency and throughput (elastic#88452)
  fix copy (elastic#88481)
  ...
banderror pushed a commit to banderror/kibana that referenced this pull request Feb 24, 2021
…lastic#88420)

## Summary

This removes some duplicate API calls to reduce pressure on the backend and speed up querying times within the application for the front end. This fixes some of the issues of elastic#82327, but there are several performance improvements that are going to be needed to help reduce the slowness when you have a system under a lot of pressure.

So far this removes duplication for these API calls when you are on the manage detection rules page:

```ts
api/detection_engine/rules/_find
api/detection_engine/rules/_find_statuses
api/detection_engine/tags
```

<img width="2465" alt="Screen Shot 2021-01-14 at 3 53 21 PM" src="https://user-images.githubusercontent.com/1151048/104662295-c031e080-5687-11eb-92d7-18b9ad355646.png">

* This hides the tags and searches while the page is loading to avoid duplicate calls when the pre-packaged rules counts come back
* This untangles the refetchRules from the refetchPrePackagedRulesStatus as two separate calls to avoid issues we have with re-rendering and re-calling the backend.
 
### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants