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

Make all risk score decimal places consistent #198450

Merged
merged 20 commits into from
Nov 8, 2024
Merged

Conversation

CAWilson94
Copy link
Contributor

@CAWilson94 CAWilson94 commented Oct 30, 2024

Summary

This PR updates risk scores to use formatter to 2DP instead of basic Math.round for consistency and accuracy.

image

image

image

With reverted alert score, showing user and host risk scores still have formatting:

image

@CAWilson94 CAWilson94 marked this pull request as ready for review November 5, 2024 10:37
@CAWilson94 CAWilson94 requested review from a team as code owners November 5, 2024 10:37
@CAWilson94 CAWilson94 requested a review from machadoum November 5, 2024 10:37
@CAWilson94 CAWilson94 self-assigned this Nov 5, 2024
@CAWilson94 CAWilson94 added the Team:Entity Analytics Security Entity Analytics Team label Nov 5, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-entity-analytics (Team:Entity Analytics)

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Hey! Thank for fixing it 🙇

I left a couple of comments. Could you please take a look?

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

I feel like we should also use the formatRiskScore function in the alert details flyout also? (see here).
Wdyt?

The flyout is accessed by pressing on the double arrow icon on the alerts table
Screenshot 2024-11-05 at 7 32 39 AM

@machadoum
Copy link
Member

@PhilippeOberti @CAWilson94 I don't think we should format the Alert risk score with formatRiskScore in the scope of this PR. This PR goal is to standardize how we display the risk score calculated by the Risk Engine. Even though it makes sense to be consistent, the alert risk score is not calculated by the Risk Engine; it is maintained by other teams, and the value is displayed in several places around the app. We should start a conversation with the affected teams before changing how it is displayed.

@CAWilson94
Copy link
Contributor Author

@machadoum this is the same for alerts page and the flyout - the risk score here is coming from the search strategy functions which seem to utilise the field API. So, for both cases under alerts, I can revert that risk score formatting change. I think that makes sense.

My note here is partially confirmation of findings for my own understanding. 😁

@CAWilson94
Copy link
Contributor Author

@machadoum @PhilippeOberti Hello, updated to only format risk score on host and user entities, reverted the alert risk score changes. Thanks! :)

Copy link
Contributor

@PhilippeOberti PhilippeOberti 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 making these changes @CAWilson94. The @elastic/security-threat-hunting-investigations team is no longer a code owner here so my approval has no value! 😆

I see that some ESS and serverless Cypress test are failing, feel free to reach out if you need any help troubleshooting these!

@CAWilson94 CAWilson94 added v9.0.0 v8.16.0 release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Nov 7, 2024
Copy link
Contributor

@tiansivive tiansivive 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 looking into this! From reading the other comments all looks good now 🚢

@CAWilson94 CAWilson94 removed the v8.16.0 label Nov 8, 2024
@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #89 / @ess @serverless SecuritySolution Explore Network Tls Test with Packetbeat Tls Overview Test Ensure data is returned for FlowTarget.Source
  • [job] [logs] Jest Integration Tests #5 / Http1BasePathProxyServer shouldRedirect it will NOT redirect if it is not a GET verb

Metrics [docs]

✅ unchanged

History

cc @CAWilson94

@CAWilson94 CAWilson94 merged commit 4b6b1c3 into elastic:main Nov 8, 2024
47 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11745911633

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 8, 2024
## Summary

This PR updates risk scores to use formatter to 2DP instead of basic
Math.round for consistency and accuracy.

![image](https://github.com/user-attachments/assets/ede600ef-acd3-463b-8f0f-8f527271836e)

![image](https://github.com/user-attachments/assets/a49f2a01-e05a-4077-8397-cff18da7cfa5)

![image](https://github.com/user-attachments/assets/599d3bcb-118f-4e32-94e6-ff4aa0a60fa8)

With reverted alert score, showing user and host risk scores still have
formatting:

![image](https://github.com/user-attachments/assets/94cb4d9e-a468-4cb7-b162-74762b134436)

(cherry picked from commit 4b6b1c3)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Nov 8, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [Make all risk score decimal places consistent
(#198450)](#198450)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Charlotte Alexandra
Wilson","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-08T16:33:50Z","message":"Make
all risk score decimal places consistent (#198450)\n\n##
Summary\r\n\r\nThis PR updates risk scores to use formatter to 2DP
instead of basic\r\nMath.round for consistency and
accuracy.\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/ede600ef-acd3-463b-8f0f-8f527271836e)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/a49f2a01-e05a-4077-8397-cff18da7cfa5)\r\n\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/599d3bcb-118f-4e32-94e6-ff4aa0a60fa8)\r\n\r\nWith
reverted alert score, showing user and host risk scores still
have\r\nformatting:\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/94cb4d9e-a468-4cb7-b162-74762b134436)","sha":"4b6b1c3effaf9918f8332a21af42409092fd8ed5","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Theme:
entity_analytics","Team:Entity
Analytics","backport:version","v8.17.0"],"title":"Make all risk score
decimal places
consistent","number":198450,"url":"https://github.com/elastic/kibana/pull/198450","mergeCommit":{"message":"Make
all risk score decimal places consistent (#198450)\n\n##
Summary\r\n\r\nThis PR updates risk scores to use formatter to 2DP
instead of basic\r\nMath.round for consistency and
accuracy.\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/ede600ef-acd3-463b-8f0f-8f527271836e)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/a49f2a01-e05a-4077-8397-cff18da7cfa5)\r\n\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/599d3bcb-118f-4e32-94e6-ff4aa0a60fa8)\r\n\r\nWith
reverted alert score, showing user and host risk scores still
have\r\nformatting:\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/94cb4d9e-a468-4cb7-b162-74762b134436)","sha":"4b6b1c3effaf9918f8332a21af42409092fd8ed5"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198450","number":198450,"mergeCommit":{"message":"Make
all risk score decimal places consistent (#198450)\n\n##
Summary\r\n\r\nThis PR updates risk scores to use formatter to 2DP
instead of basic\r\nMath.round for consistency and
accuracy.\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/ede600ef-acd3-463b-8f0f-8f527271836e)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/a49f2a01-e05a-4077-8397-cff18da7cfa5)\r\n\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/599d3bcb-118f-4e32-94e6-ff4aa0a60fa8)\r\n\r\nWith
reverted alert score, showing user and host risk scores still
have\r\nformatting:\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/94cb4d9e-a468-4cb7-b162-74762b134436)","sha":"4b6b1c3effaf9918f8332a21af42409092fd8ed5"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Charlotte Alexandra Wilson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes Team:Entity Analytics Security Entity Analytics Team Theme: entity_analytics v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants