-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[RAM] Add alert summary API #146709
[RAM] Add alert summary API #146709
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
When I include
But when I omit the
I did a little digging and it appears the index it's trying to use with the |
I think the way we are querying the "recovered alerts" is wrong. When an alert recovers the recovery should only show up in the bucket it recovered based on I think we want something like this: Where 200 alerts were active from 2:12 pm to 2:16 pm and then at 2:16 pm all 200 alerts recovered. Here is how I modified the recovered alerts query for the second chart: POST .alerts-*/_search
{
"aggs": {
"recovered_alerts_bucket": {
"date_histogram": {
"field": "kibana.alert.end",
"fixed_interval": "1m",
"extended_bounds": {
"min": "2022-11-30T20:46:52.631Z",
"max": "2022-11-30T21:46:52.631Z"
}
}
}
},
"query": {
"bool": {
"filter": [
{
"range": {
"kibana.alert.end": {
"gt": "2022-11-30T20:46:52.631Z",
"lt": "2022-11-30T21:46:52.631Z"
}
}
},
{
"term": {
"kibana.alert.status": "recovered"
}
}
]
}
},
"size": 0
} @vinaychandrasekhar @maciejforcone Am I interpreting the design correctly? |
I also think that second makes more sense. User needs to see the duration of active alerts (so it should be a timerange to use for further investigation), where for recovered we should use just a moment of recovery (so it's a timestamp, when all went back to normal). And for the chart type, are we moving from line to area? I wanted us to use the same visual pattern that we have in APM, so line = recent timerange, area = previous timerange, for the comparison use case. |
@simianhacker , we do not need the |
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.
Kibana Platform Security changes LGTM 👍
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.
When I was checking the result, the key_as_string
was not the same for active and recovered:
Update: After a discussion with @XavierM, I learned that this format is returned from the related query and we can use key
in our charts instead. So this difference will be kept as it is.
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
), | ||
}, | ||
options: { | ||
tags: ['access:rac'], |
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.
What does this option do?
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 is for kibana security, I think we created a special rac
access.
const { gte, lte, featureIds, filter, fixed_interval: fixedInterval, index } = request.body; | ||
if ( | ||
!( | ||
moment(gte, 'YYYY-MM-DDTHH:mm:ss.SSSZ', true).isValid() && |
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.
Can we use DateFromString instead to validate it at the body validation level?
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.
I only imagined that will use this UTC format and not just a date. However, if you feel that we should allow just date without time, I can change it back to moment.ISO_8601
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.
No, it is fine, I also used the same time format in my PR for the related component.
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.
I wasn't able to check the graphs as was shown by @simianhacker, so I'll wait for his approval from our team.
I wasn't able to check the graphs as was shown by @simianhacker, so I'll wait for his approval from our team.
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.
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/rule_registry/server/routes/get_alert_summary.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
throw Boom.badRequest('gte and/or lte are not following the UTC format'); | ||
} | ||
|
||
if (fixedInterval && fixedInterval?.match(/^\d{1,2}['m','h','d','w']$/) == null) { |
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.
Should we validate this with a custom validator on the body params?
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.
I did not find an easy way to do it with io-ts, so I will keep it this way for now.
x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts
Outdated
Show resolved
Hide resolved
x-pack/test/rule_registry/security_and_spaces/tests/basic/index.ts
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.
LGTM
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Resolve: #141487
Checklist