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] Adds logging and performance fan out API for threat/Indicator matching #82546

Merged

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Nov 3, 2020

Summary

  • Adds logging output for trouble shooting
  • Adds an API to be able to configure how many concurrent searches and how many items per search to use

API additions are these two switches:

concurrent_searches
items_per_search

When you create a rule. You can use the following example to post one or to change the settings to see the performance impact:

./post_rule.sh ./rules/queries/query_with_threat_mapping_perf.json

Without using these two experimental API settings, the functionality is the same as the existing algorithm and only advanced users will be able to set the additional REST settings through this API. If you use the front end after setting the settings, the settings will be reset as that's how the forms code currently works and this will not preserve the settings if afterwards a rule is edited/changed.

Both these API settings should be considered experimental and potentially breakable as we figure out the best performance strategies for indicator matching.

Checklist

@FrankHassanabad FrankHassanabad self-assigned this Nov 3, 2020
@FrankHassanabad FrankHassanabad added v7.11.0 v8.0.0 Feature:Detection Rules Anything related to Security Solution's Detection Rules Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Nov 5, 2020
@FrankHassanabad FrankHassanabad marked this pull request as ready for review November 5, 2020 22:41
@FrankHassanabad FrankHassanabad requested review from a team as code owners November 5, 2020 22:41
@@ -504,7 +504,7 @@ describe('rules_notification_alert_type', () => {
await alert.executor(payload);
expect(logger.error).toHaveBeenCalled();
expect(logger.error.mock.calls[0][0]).toContain(
'An error occurred during rule execution: message: "Threat Match rule is missing threatQuery and/or threatIndex and/or threatMapping: threatQuery: "undefined" threatIndex: "undefined" threatMapping: "undefined"" name: "Detect Root/Admin Users" id: "04128c15-0d1b-4716-a4c5-46997ac7f3bd" rule id: "rule-1" signals index: ".siem-signals"'
'An error occurred during rule execution: message: "Indicator match is missing threatQuery and/or threatIndex and/or threatMapping: threatQuery: "undefined" threatIndex: "undefined" threatMapping: "undefined"" name: "Detect Root/Admin Users" id: "04128c15-0d1b-4716-a4c5-46997ac7f3bd" rule id: "rule-1" signals index: ".siem-signals"'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the wording here match the UI when it comes to threatIndex, threatMapping, threatQuery or at least let the user know that's what they correspond to? Or I guess that's particular to the field name right? Would this be an error the user sees in the front end ever?

Copy link
Contributor

Choose a reason for hiding this comment

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

I take back my comment, that would add a lot of overhead any time we decide to switch what we call it in the UI.

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Nov 6, 2020

Choose a reason for hiding this comment

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

I view log files as something that is for both the users and us to look through so when UI wording changes I might not be easily able to keep files and REST up to date (or REST API would break) but for things like log file messages I try to update them to match the UI and the wording in people's heads.

I try to take extra time to make the logs more human readable for users more so than even myself even though the majority of the time they end up in the forums. I kind of would like to make them more clear.

We don't i18n them though so at the moment they're not as accessible as we would like them to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I was just thinking about whether they'd get confused with the threat vs indicator lingo in the error message. i18n eventually would be super nice too.

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.

This looks great!! I pulled down, ran tests and played around with changing the concurrent search number. The extra logs added are super helpful.

Added some nit comments and we chatted offline about the upper bound limits of these fields that I'm super curious to find out more about. This is really cool stuff!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
securitySolution 7.8MB 7.8MB +303.0B

distributable file count

id before after diff
default 42729 42730 +1

History

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

@FrankHassanabad FrankHassanabad merged commit fb8cd5b into elastic:master Nov 7, 2020
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Nov 7, 2020
…at/Indicator matching (elastic#82546)

## Summary

* Adds logging output for trouble shooting
* Adds an API to be able to configure how many concurrent searches and how many items per search to use

API additions are these two switches:

```
concurrent_searches
items_per_search
```

When you create a rule. You can use the following example to post one or to change the settings to see the performance impact:

```ts
./post_rule.sh ./rules/queries/query_with_threat_mapping_perf.json
```

Without using these two experimental API settings, the functionality is the same as the existing algorithm and only advanced users will be able to set the additional REST settings through this API. If you use the front end after setting the settings, the settings will be reset as that's how the forms code currently works and this will not preserve the settings if afterwards a rule is edited/changed.

Both these API settings should be considered experimental and potentially breakable as we figure out the best performance strategies for indicator matching.

### 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 Nov 7, 2020
…at/Indicator matching (#82546) (#82911)

## Summary

* Adds logging output for trouble shooting
* Adds an API to be able to configure how many concurrent searches and how many items per search to use

API additions are these two switches:

```
concurrent_searches
items_per_search
```

When you create a rule. You can use the following example to post one or to change the settings to see the performance impact:

```ts
./post_rule.sh ./rules/queries/query_with_threat_mapping_perf.json
```

Without using these two experimental API settings, the functionality is the same as the existing algorithm and only advanced users will be able to set the additional REST settings through this API. If you use the front end after setting the settings, the settings will be reset as that's how the forms code currently works and this will not preserve the settings if afterwards a rule is edited/changed.

Both these API settings should be considered experimental and potentially breakable as we figure out the best performance strategies for indicator matching.

### 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
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 9, 2020
* master: (68 commits)
  [Fleet] Make stream id unique in agent policy (elastic#82447)
  skip flaky suite (elastic#82915)
  skip flaky suite (elastic#75794)
  Copy `dateAsStringRt` to observability plugin (elastic#82839)
  [Maps] rename connected_components/map folder to mb_map (elastic#82897)
  [Security Solution] Fix EventsViewer DnD cypress tests (elastic#82619)
  [Security Solution] Adds logging and performance fan out API for threat/Indicator matching (elastic#82546)
  Implemented Alerting health status pusher by using task manager and status pooler for Kibana status plugins 'kibanahost/api/status' (elastic#79056)
  [APM] Adds new configuration 'xpack.apm.maxServiceEnvironments' (elastic#82090)
  Move single use function in line (elastic#82885)
  [ML] Add unsigned_long support to data frame analytics and anomaly detection (elastic#82636)
  Add flot_chart dependency from shared_deps to Shareable Runtime (elastic#81649)
  [Security Solution][Detections] - Auto refresh all rules/monitoring tables (elastic#82062)
  [APM] Fix apm e2e runner script commands (elastic#82798)
  [Ingest Manager] Move cache functions to from registry to archive (elastic#82871)
  Update webpack-dev-server and webpack-cli (elastic#82844)
  [Uptime] Migrate to new es client (elastic#82003)
  Move parseAndVerify* functions to validation.ts (elastic#82845)
  Remove yeoman & yo (elastic#82825)
  [Canvas] Fix elements not being updated properly when filter is changed on workpad (elastic#81863)
  ...
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 10, 2020
…e-details-overlay

* 'master' of github.com:elastic/kibana: (201 commits)
  Added `defaultActionMessage` to index threshold alert UI type definition (elastic#80936)
  [ILM] Migrate Delete phase and name field to Form Lib (elastic#82834)
  skip flaky suite (elastic#57426)
  [Alerting] adds an Run When field in the alert flyout to assign the action to an Action Group (elastic#82472)
  [APM] Expose APM event client as part of plugin contract (elastic#82724)
  [Logs UI] Fix errors during navigation (elastic#78319)
  Enable send to background in TSVB (elastic#82835)
  SavedObjects search_dsl: add match_phrase_prefix clauses when using prefix search (elastic#82693)
  [Ingest Manager] Unify install* under installPackage (elastic#82916)
  [Fleet] Make stream id unique in agent policy (elastic#82447)
  skip flaky suite (elastic#82915)
  skip flaky suite (elastic#75794)
  Copy `dateAsStringRt` to observability plugin (elastic#82839)
  [Maps] rename connected_components/map folder to mb_map (elastic#82897)
  [Security Solution] Fix EventsViewer DnD cypress tests (elastic#82619)
  [Security Solution] Adds logging and performance fan out API for threat/Indicator matching (elastic#82546)
  Implemented Alerting health status pusher by using task manager and status pooler for Kibana status plugins 'kibanahost/api/status' (elastic#79056)
  [APM] Adds new configuration 'xpack.apm.maxServiceEnvironments' (elastic#82090)
  Move single use function in line (elastic#82885)
  [ML] Add unsigned_long support to data frame analytics and anomaly detection (elastic#82636)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Anything related to Security Solution's Detection Rules release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants