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] Create rule from timeline #143020
[Security Solution] Create rule from timeline #143020
Conversation
@elasticmachine merge upstream |
(timeline: TimelineModel) => { | ||
setLoadingTimeline(false); | ||
const newQuery = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic gets moved to new hook, use_rule_from_timeline.tsx
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
SourcererScopeName.timeline | ||
); | ||
|
||
const [ogDataView] = useState({ dataViewId, selectedPatterns }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this in useState if the update function is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also consider renaming to just originalDataView or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this way when dataViewId and selectedPatterns update, ogDataView
will stay the original values. Do you know another way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good use case for useRef
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed it tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamster10 useRef
‘s current
property is mutable, but useState
‘s state variable not. The React team recommends in the documentation for setState, treat state like an immutable variable.
useRef
is meant to be mutable and useState
is meant to be immutable. Either would work, but as I do not want these values to ever update I think useState
without pulling in the update method is the correct implementation
...rity_solution/public/detections/containers/detection_engine/rules/use_rule_from_timeline.tsx
Outdated
Show resolved
Hide resolved
...rity_solution/public/detections/containers/detection_engine/rules/use_rule_from_timeline.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephmilovic rules area related changes look good. I left some non critical comments.
x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx
Outdated
Show resolved
Hide resolved
...lugins/security_solution/public/detections/containers/detection_engine/rules/translations.ts
Outdated
Show resolved
Hide resolved
...lugins/security_solution/public/detections/containers/detection_engine/rules/translations.ts
Outdated
Show resolved
Hide resolved
@@ -133,6 +134,7 @@ describe('Custom query rules', () => { | |||
|
|||
cy.log('Filling define section'); | |||
importSavedQuery(this.timelineId); | |||
removeAlertsIndex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation here. I worry that the information you shared here can be forgotten in the future so it won't be so clear why alerts index is cleared here. Is it possible to remove alerts index inside importSavedQuery()
? If not I'd leave a short comment in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rules Area LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing the small nits, love this feature! Once upon an onweek I did something similar, only for eql only, could be cool to add that on top of this eventually. lgtm though 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This is an awesome feature!:tada:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alerts area changes LGTM
useRuleFromTimeline
Cypress
Tests that use the feature "Import query from saved timeline" now have a different index pattern. We now match exactly the index pattern to the timeline index pattern. So:
getIndexPatterns().join('')
becomes'auditbeat-*'
. Depending on if any alerts have been generated yet, the timeline index pattern could also include.alerts-security.alerts-default
. Added a methodremoveAlertsIndex
to check if alerts index exists and remove it if it does, or it will break tests.How to test
ho_ho_ho
host.name: *
, the other one uses theho_ho_ho
data view and queriesgood.job: *
.ho_ho_ho
timeline and select "Create rule from timeline"