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] Remove the advanced sorting switch from the rules management page #149840

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Jan 30, 2023

Resolves: #138907
Resolves: #148196

Summary

  • Added missing outcomeOrder field to the alerting framework
  • Removed the advanced sorting switch from the rules management page
  • Both rules management and rules monitoring tables now have all columns sortable, including indexing time, query time, last gap, and last response
  • Sorting now happens entirely on the server side. No in-memory structures are needed.
  • Removed the version column as intended in [Security Solution] Remove version column from the Rules table #148196

There's one caveat, though. The running status is represented as a different rule field. So when sorting by the last status, if there are any "running" rules, they could appear in any row on the table:

Screenshot 2023-01-31 at 16 18 29

@xcrzx xcrzx added release_note:enhancement Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team v8.7.0 labels Jan 30, 2023
@xcrzx xcrzx self-assigned this Jan 30, 2023
@xcrzx xcrzx force-pushed the remove-in-memory-table branch 11 times, most recently from 6971bd6 to cd5e413 Compare February 1, 2023 15:02
@xcrzx xcrzx marked this pull request as ready for review February 1, 2023 16:55
@xcrzx xcrzx requested review from a team as code owners February 1, 2023 16:55
@xcrzx xcrzx requested a review from spong February 1, 2023 16:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx force-pushed the remove-in-memory-table branch 4 times, most recently from 86811f1 to 1375e45 Compare February 2, 2023 15:26
...doc.attributes,
lastRun: {
...doc.attributes.lastRun,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall off hand, but are there any old legacy outcome's that a user could have lingering around that wouldn't be included in the RuleLastRunOutcomeOrderMap? If so, is undefined fine here until the next rule execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outcome field was added in 8.6 with only three values specified in the RuleLastRunOutcomeOrderMap. So I don't expect any other values there. That said, undefined should also work just fine.

Copy link
Member

Choose a reason for hiding this comment

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

Problem is, the outcomeOrder is defined as a number, but in the case of a outcome: undefined - or some change in the future that would add a new outcome but forget to add the order - the order will end up being undefined. Feels like we should change outcomeOrder to be number | undefined, or write a function to generate the updated value that deals with the potential undefined value, where it's generated in execution.

If we want to keep it typed as a number, then I think we need to supply a number here, but not sure what we'd use. If we go with making outcome_order as number | undefined, then this code is fine as-is , as it would generate an undefined (if outcome is not set or an unexpected value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I miss something, outcome cannot be undefined. See the 8.6 migration, for example:
https://github.com/elastic/kibana/blob/2d675b47a7c3210a1b430e89ff51c9d7485d1fd2/x-pack/plugins/alerting/server/saved_objects/migrations/8.6/index.ts#L22-L35

If the outcome is unknown, the whole lastRun structure is omitted.

Furthermore, adding a new outcome value will cause a type error if the value is not added to the outcome->order map:

Screenshot 2023-02-07 at 10 12 49

So TypeScript protects us from forgetting to add values to the linked structures.

That said, if you feel strongly about changing the type of outcomeOrder to number | undefined, I'm okay with that. It doesn't affect the code of Security Solution in any way, as we don't work with that field directly.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! We've become more sensitive to migrations recently so wanted to understand this one well.

The main thing I'm worrying about is some bug where we would unintentionally have outcome set to some invalid value, as part of normal (presumably buggy!) operation. And then the migration would end up writing out an undefined for the order. But ... that's valid for that field :-)

So, even in that case, I don't think there's anything else we can do to improve the migration.

The rest of the type-checking on outcome looks good, thanks for explaining.

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, tested locally, code reviewed and rules area changes LGTM! 👍

One issue with sorting on Version, but I think we can just remove that column (as intended in #148196) and call it a day.

As always, super clean work here @xcrzx! Thank you for seeing this feature full-circle -- the in-memory table served our users well in the interim, and it's now nice to have mapped_params support where we need it, awesome! 😀

@xcrzx xcrzx force-pushed the remove-in-memory-table branch 2 times, most recently from ce10b97 to 2d675b4 Compare February 6, 2023 12:01
Copy link
Contributor

@nkhristinin nkhristinin left a comment

Choose a reason for hiding this comment

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

security-solution-platform changes LGTM!

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.

Awesome, reviewed the diff and tested the PR locally. LGTM 👍

I left a bunch of non-blocking comments that can be addressed in a follow-up one, if needed.

Comment on lines 13 to 28
export const SortField = t.union([
t.literal('created_at'),
t.literal('createdAt'), // Legacy notation, keeping for backwards compatibility
t.literal('enabled'),
t.literal('execution_summary.last_execution.date'),
t.literal('execution_summary.last_execution.metrics.execution_gap_duration_s'),
t.literal('execution_summary.last_execution.metrics.total_indexing_duration_ms'),
t.literal('execution_summary.last_execution.metrics.total_search_duration_ms'),
t.literal('execution_summary.last_execution.status'),
t.literal('name'),
t.literal('risk_score'),
t.literal('riskScore'), // Legacy notation, keeping for backwards compatibility
t.literal('severity'),
t.literal('updated_at'),
t.literal('updatedAt'), // Legacy notation, keeping for backwards compatibility
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not forget to update the docs with this info. I can open a ticket if you haven't already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is a type that applies specifically to the find rules API endpoint, but it's not common. Let's rename it to FindRulesSortField and move it to x-pack/plugins/security_solution/common/detection_engine/rule_management/api/rules/find_rules/request_schema.ts

},
{
refetchInterval: isRefreshOn && !isActionInProgress && autoRefreshSettings.value,
keepPreviousData: true, // Use this option so that the state doesn't jump between "success" and "loading" on page change
Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciate the comment!

import { assertUnreachable } from '../../../../../../common/utility_types';

/**
* Transform the sort field name from the request to the Alerting framework
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for all these comments, it's a ton of help for our future self or anyone new to this codebase.

I'm thinking now: what about filtering? We should probably do the same for filters as well, right?

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

reviewed alerting framework changes; I think we need to do something about the order potentially being undefined, and there are notes to add some additional tests. Other than that, LGTM

x-pack/plugins/alerting/common/rule.ts Show resolved Hide resolved
...doc.attributes,
lastRun: {
...doc.attributes.lastRun,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
Copy link
Member

Choose a reason for hiding this comment

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

Problem is, the outcomeOrder is defined as a number, but in the case of a outcome: undefined - or some change in the future that would add a new outcome but forget to add the order - the order will end up being undefined. Feels like we should change outcomeOrder to be number | undefined, or write a function to generate the updated value that deals with the potential undefined value, where it's generated in execution.

If we want to keep it typed as a number, then I think we need to supply a number here, but not sure what we'd use. If we go with making outcome_order as number | undefined, then this code is fine as-is , as it would generate an undefined (if outcome is not set or an unexpected value).

@@ -173,6 +173,7 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
},
outcome: 'succeeded',
outcome_msg: null,
outcome_order: 0,
Copy link
Member

Choose a reason for hiding this comment

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

We should add tests for 'warning' and 'failed' to ensure the outcome_order is expected. I think that can just be added to any other test that receives those outcomes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what tests you mean here. I've added more test cases to the migrations suite, and haven't found any explicit integrational tests for the lastRun or outcome fields. This file is the only place in x-pack/test/alerting_api_integration besides migrations where execution outcome is surfaced.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see! I figured we had some - I do see some outcome FT for the event log, but not seeing any for the rule itself.
@XavierM, did I miss some? Looks like you were involved in some of this work in a previous PR. Seems like we should add some (in a separate issue/PR) if we don't have any.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we will add specific test around that in the near future. We will use the same one for executionStatus if they exist

Copy link
Contributor

Choose a reason for hiding this comment

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

They do exist! So here a ticket #150487 to add them for last_run

@xcrzx xcrzx force-pushed the remove-in-memory-table branch from 2d675b4 to c67e600 Compare February 7, 2023 10:14
@xcrzx xcrzx requested a review from pmuellr February 7, 2023 10:14
@banderror
Copy link
Contributor

@pmuellr @xcrzx My 2 cents: let's reserve order = 0 for outcome = undefined, and start the real values of the order from 10:

export const RuleLastRunOutcomeOrderMap: Record<RuleLastRunOutcomes, number> = {
  succeeded: 10,
  warning: 20,
  failed: 30,
};

Having nullable fields makes the code more error-prone, and here it seems we could avoid that.

@xcrzx xcrzx force-pushed the remove-in-memory-table branch from c67e600 to c61110b Compare February 7, 2023 10:43
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3690 3710 +20

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
alerting 461 466 +5

Async chunks

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

id before after diff
securitySolution 13.7MB 13.7MB -2.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 40.9KB 41.0KB +127.0B
triggersActionsUi 117.8KB 117.8KB +31.0B
total +158.0B
Unknown metric groups

API count

id before after diff
alerting 471 476 +5

History

  • 💔 Build #106003 failed c67e600af82064117c4e075b7b2a29eda3215f64
  • 💛 Build #105605 was flaky 2d675b47a7c3210a1b430e89ff51c9d7485d1fd2
  • 💚 Build #105158 succeeded 1375e4559a469bfcc6fe56c021820392e68119bf
  • 💔 Build #105023 failed f14ae695e59cd89dd05b5605cb313e4fb9c085e7
  • 💔 Build #104736 failed cd5e4135fead98e09e36bef0ce86335c0effc456

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

cc @xcrzx

@xcrzx
Copy link
Contributor Author

xcrzx commented Feb 7, 2023

@pmuellr @xcrzx My 2 cents: let's reserve order = 0 for outcome = undefined, and start the real values of the order from 10

As I mentioned in my comment above, there normally shouldn't be a situation when outcome or outcomeOrder is undefined. So if you see how that could happen, I'd prefer to fix the root cause rather than adding workarounds.

Copy link
Member

@pmuellr pmuellr 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 explaining, LGTM

@xcrzx xcrzx merged commit 4513f7b into elastic:main Feb 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 7, 2023
@xcrzx xcrzx deleted the remove-in-memory-table branch February 8, 2023 16:00
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 Feature:Rule Management Security Solution Detection Rule Management area release_note:enhancement Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.7.0
Projects
None yet
9 participants