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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Detections] Updates Indexing/Query Time columns in Rule Monitoring table to be SUM instead of MAX #114023

Merged
merged 10 commits into from
Oct 8, 2021
Merged

[Security Solution][Detections] Updates Indexing/Query Time columns in Rule Monitoring table to be SUM instead of MAX #114023

merged 10 commits into from
Oct 8, 2021

Conversation

spong
Copy link
Member

@spong spong commented Oct 5, 2021 •
edited
Loading

Summary

Updates the Indexing Time & Query Time columns in the Rule Monitoring table to be SUM instead of MAX, thus showing the total duration of indexing/querying phases within a Rule's execution rather than just the phase that took the longest. Also adds tooltips to columns for better understanding these metrics.

Note: Wanted to add a link to documentation for Last Gap column, but cannot add links within EuiToolTip and didn't want to mis-align design of other column tooltips by introducing a popover. @elastic/security-design please advise on desired action or copy changes here -- thanks! 🙂

Update: As guided by design, changed Last Gap tooltip to be EuiPopover and added link to docs.

Indexing Time:

Query Time:

Last Gap:

Checklist

Delete any items that are not applicable to this PR.

@spong spong added bug Fixes for quality problems that affect the customer experience release_note:fix v8.0.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.16.0 Feature:Rule Monitoring Security Solution Detection Rule Monitoring area Team:Detection Rule Management Security Detection Rule Management Team labels Oct 5, 2021
@spong spong self-assigned this Oct 5, 2021
@spong spong requested a review from a team as a code owner October 5, 2021 22:36
@elasticmachine
Copy link
Contributor

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

@spong
Copy link
Member Author

spong commented Oct 6, 2021

@elasticmachine merge upstream

@spong spong added the UX: UI/UX Consultation Requires UX lead input/consult before development and UX lead approval on PR before merge. label Oct 6, 2021
xcrzx
xcrzx approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@xcrzx xcrzx 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 updating the columns, @spong 👍
Added a nit about using the existing sum function, but all else LGTM!

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/columns.tsx Outdated Show resolved Hide resolved
@yiyangliu9286
Copy link

yiyangliu9286 commented Oct 7, 2021 •
edited
Loading

@spong according to eui (comment)

If you need interactive content or anything other than text, we recommend using EuiPopover instead.

I have updated the Last Gap column from using EuiToolTip to EuiPopover instead. Here is the mock for this (figma):

Please note that I did a quick update to suggest updating the colour for the question icon for Last gap (if any) column to be the primary blue as an indication that this is an icon button that users can interact with and distinguish this between the other two columns which use hover on EuiTooltip.

Security Rules_Last gap tooltip

@spong spong requested a review from a team as a code owner October 7, 2021 23:32
TinaHeiligers
TinaHeiligers approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

DocLinks changes LGTM

@spong spong enabled auto-merge (squash) October 8, 2021 00:44
@spong spong added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 8, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2300 2301 +1

Async chunks

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

id before after diff
securitySolution 4.3MB 4.3MB +1.8KB

Page load bundle

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

id before after diff
core 300.4KB 300.4KB -65.0B

History

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

cc @spong

@spong spong merged commit 15c7bd0 into elastic:master Oct 8, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 8, 2021
@spong @kibanamachine
[Security Solution][Detections] Updates Indexing/Query Time columns i…
da520cd 
…n Rule Monitoring table to be SUM instead of MAX (elastic#114023)

## Summary

Updates the `Indexing Time` & `Query Time` columns in the `Rule Monitoring` table to be `SUM` instead of `MAX`, thus showing the total duration of indexing/querying phases within a Rule's execution rather than just the phase that took the longest. Also adds tooltips to columns for better understanding these metrics.

~Note: Wanted to add a link to documentation for `Last Gap` column, but cannot add links within `EuiToolTip` and didn't want to mis-align design of other column tooltips by introducing a popover. @elastic/security-design please advise on desired action or copy changes here -- thanks!~ 🙂 

Update: As guided by design, changed `Last Gap` tooltip to be `EuiPopover` and added link to docs.

##### Indexing Time:
<p align="center">
  <img width="700" src="https://user-images.githubusercontent.com/2946766/136475361-cedd7c6a-6a0e-4a86-8467-c929aed0f16e.png" />
</p>


##### Query Time:
<p align="center">
  <img width="700" src="https://user-images.githubusercontent.com/2946766/136475378-1228dfcf-a921-4c0e-8f1e-7594e9c220d4.png" />
</p>

##### Last Gap:
<p align="center">
  <img width="700" src="https://user-images.githubusercontent.com/2946766/136475412-b54f2419-ced8-43d8-8643-09c8e2cacc44.png" />
</p>



### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
@kibanamachine kibanamachine mentioned this pull request Oct 8, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 12, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Oct 13, 2021
@kibanamachine @spong
[Security Solution][Detections] Updates Indexing/Query Time columns i…
460d241 
…n Rule Monitoring table to be SUM instead of MAX (#114023) (#114380)

## Summary

Updates the `Indexing Time` & `Query Time` columns in the `Rule Monitoring` table to be `SUM` instead of `MAX`, thus showing the total duration of indexing/querying phases within a Rule's execution rather than just the phase that took the longest. Also adds tooltips to columns for better understanding these metrics.

~Note: Wanted to add a link to documentation for `Last Gap` column, but cannot add links within `EuiToolTip` and didn't want to mis-align design of other column tooltips by introducing a popover. @elastic/security-design please advise on desired action or copy changes here -- thanks!~ 🙂 

Update: As guided by design, changed `Last Gap` tooltip to be `EuiPopover` and added link to docs.

##### Indexing Time:
<p align="center">
  <img width="700" src="https://user-images.githubusercontent.com/2946766/136475361-cedd7c6a-6a0e-4a86-8467-c929aed0f16e.png" />
</p>


##### Query Time:
<p align="center">
  <img width="700" src="https://user-images.githubusercontent.com/2946766/136475378-1228dfcf-a921-4c0e-8f1e-7594e9c220d4.png" />
</p>

##### Last Gap:
<p align="center">
  <img width="700" src="https://user-images.githubusercontent.com/2946766/136475412-b54f2419-ced8-43d8-8643-09c8e2cacc44.png" />
</p>



### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 13, 2021
@yiyangliu9286 yiyangliu9286 mentioned this pull request Nov 3, 2021
Closed
1 task
@banderror banderror mentioned this pull request Nov 11, 2021
Open
19 tasks
@spong spong mentioned this pull request Dec 8, 2021
Closed
@peluja1012 peluja1012 mentioned this pull request Mar 3, 2022
Closed
@spong spong deleted the rule-monitoring-column-updates branch April 12, 2022 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@xcrzx xcrzx xcrzx approved these changes

Assignees

@spong spong

Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:Rule Monitoring Security Solution Detection Rule Monitoring area release_note:fix Team:Detection Rule Management Security Detection Rule Management Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. UX: UI/UX Consultation Requires UX lead input/consult before development and UX lead approval on PR before merge. v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@spong @elasticmachine @yiyangliu9286 @kibanamachine @xcrzx @TinaHeiligers