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

[RAC] ALerts table in observability #103270

Merged
merged 15 commits into from
Jul 6, 2021

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Jun 24, 2021

Closes #98611

Summary

Add alerts table in Observability =>

image

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@XavierM XavierM added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Jun 24, 2021
@XavierM XavierM requested a review from a team as a code owner June 24, 2021 13:12
@XavierM XavierM added the Team:Threat Hunting Security Solution Threat Hunting Team label Jun 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@XavierM XavierM requested a review from a team as a code owner June 30, 2021 14:27
@katrin-freihofner
Copy link
Contributor

katrin-freihofner commented Jul 1, 2021

UX improvements

  • Duration column should be right-aligned.
  • Units in the duration column should be abbreviated
  • Severity should not be unknown (suggestions to use critical if unknown)
  • The reason should use up the full width available
  • Actions (very left column) should have a headline Actions
  • The number of rows and pagination is not as designed cc @mdefazio
  • Do we need the experimental badge and callout?
  • I don't think the placeholder text in the search box is a great example of what to search for

cc @jasonrhodes @cyrille-leclerc @weltenwort @mgiota

@XavierM
Copy link
Contributor Author

XavierM commented Jul 1, 2021

UX improvements

  • Duration column should be right-aligned.
  • Units in the duration column should be abbreviated
  • Severity should not be unknown (suggestions to use critical if unknown)
  • The reason should use up the full width available
  • Actions (very left column) should have a headline Actions
  • The number of rows and pagination is not as designed cc @mdefazio
  • Do we need the experimental badge and callout?
  • I don't think the placeholder text in the search box is a great example of what to search for

cc @jasonrhodes @cyrille-leclerc @weltenwort @mgiota

Noted ❤️ , but this PR is not the final product because we are still using t-grid and the team are in work to convert t-grid with EuiDatagrid. This PR will help to see integration/improvement on the Alert table when it is merged with new updates.

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

The All view isn't showing any alerts when it's selected:

all-view

float: t.number,
scaled_float: t.number,
unsigned_long: t.number,
boolean: t.union([t.number, BooleanFromString]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would using t.boolean instead of t.number, e.g:

t.union([t.boolean, BooleanFromString])

make sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bad copy pasta, thank you!

@XavierM
Copy link
Contributor Author

XavierM commented Jul 1, 2021

The All view isn't showing any alerts when it's selected:

That's correct, I forget to remove the all button. In one of the RAC UX experience, they decided to not have the all functionality to simplify the bulk update alerts.

rowCellRender: ({ data }: ActionProps) => {
const dataFieldEs = data.reduce((acc, d) => ({ ...acc, [d.field]: d.value }), {});
const decoratedAlerts = decorateResponse(
[dataFieldEs] ?? [],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is the ?? [] still required here (and here)?

}
closePopover={() => setIsPopoverOpen(false)}
>
<EuiPopoverTitle>Actions</EuiPopoverTitle>
Copy link
Contributor

Choose a reason for hiding this comment

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

consider creating an i18n for this

<EuiButtonEmpty href={prepend(link ?? '')}>
<EuiFlexGroup alignItems="center" component="span" gutterSize="s">
<EuiFlexItem grow={false}>
<EuiButtonIcon aria-label="view in app" iconType="link" color="text" />
Copy link
Contributor

@andrew-goldstein andrew-goldstein Jul 1, 2021

Choose a reason for hiding this comment

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

consider using an i18n for the aria-label
EDIT: and here

Copy link
Contributor

@andrew-goldstein andrew-goldstein 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 moving this forward @XavierM! 🙏
left just a few minor comments
LGTM from the t-grid side

Copy link
Member

@weltenwort weltenwort 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 putting this together. As it is the table should be sufficient to check that the observability solution are poperly registering their lifecycle alerts. 👍

Given the list of UX problems already enumerated by @katrin-freihofner we wouldn't want to release it like this. But assuming the <EuiDataGrid>-based implementation is merged soon it seems wasteful to make extensive changes here.

setRefetch,
sort: [
{
columnId: '@timestamp',
Copy link
Member

Choose a reason for hiding this comment

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

This essentially means "sort by last update", which might not be intuitive to the user. How about we sort by "triggered" (kibana.rac.alert.start) instead?

Suggested change
columnId: '@timestamp',
columnId: 'kibana.rac.alert.start',

return (
<EuiButtonIcon
size="s"
target="_blank"
Copy link
Member

Choose a reason for hiding this comment

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

What's the intention behind opening in a new tab here? Shouldn't that be up to the user?

@andrew-goldstein
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 248 249 +1

Async chunks

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

id before after diff
apm 4.3MB 4.3MB +1.5KB
observability 478.9KB 483.4KB +4.5KB
securitySolution 6.3MB 6.3MB +103.0B
timelines 265.6KB 266.0KB +385.0B
total +6.4KB

Page load bundle

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

id before after diff
observability 55.3KB 54.9KB -481.0B
timelines 210.1KB 210.2KB +107.0B
total -374.0B
Unknown metric groups

References to deprecated APIs

id before after diff
observability 14 18 +4

History

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

@andrew-goldstein andrew-goldstein merged commit cf9e88c into elastic:master Jul 6, 2021
@andrew-goldstein andrew-goldstein deleted the t-grid-in-obs branch July 6, 2021 16:28
andrew-goldstein pushed a commit to andrew-goldstein/kibana that referenced this pull request Jul 6, 2021
Closes elastic#98611

## Summary

Add alerts table in Observability => 

![image](https://user-images.githubusercontent.com/189600/123854490-c68ddf00-d8ec-11eb-897e-2217249d5fba.png)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

| Risk                      | Probability | Severity | Mitigation/Notes        |
|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces&mdash;unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. |
| Multiple nodes&mdash;Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. |
| Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. |
| [See more potential risk examples](https://github.com/elastic/kibana/blob/master/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
andrew-goldstein added a commit that referenced this pull request Jul 6, 2021
Closes #98611

## Summary

Add alerts table in Observability => 

![image](https://user-images.githubusercontent.com/189600/123854490-c68ddf00-d8ec-11eb-897e-2217249d5fba.png)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

| Risk                      | Probability | Severity | Mitigation/Notes        |
|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces&mdash;unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. |
| Multiple nodes&mdash;Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. |
| Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. |
| [See more potential risk examples](https://github.com/elastic/kibana/blob/master/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com>
darnautov pushed a commit to darnautov/kibana that referenced this pull request Jul 7, 2021
Closes elastic#98611

## Summary

Add alerts table in Observability => 

![image](https://user-images.githubusercontent.com/189600/123854490-c68ddf00-d8ec-11eb-897e-2217249d5fba.png)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

| Risk                      | Probability | Severity | Mitigation/Notes        |
|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces&mdash;unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. |
| Multiple nodes&mdash;Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. |
| Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. |
| [See more potential risk examples](https://github.com/elastic/kibana/blob/master/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace basic table on alerts page with TGrid
7 participants