-
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
[AO] Add alertDetailsUrl to infra rule types #157987
[AO] Add alertDetailsUrl to infra rule types #157987
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/oblt-deploy |
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
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.
Infra Monitoring UI LGTM. Just left a non-blocking nit
x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.ts
Show resolved
Hide resolved
Co-authored-by: Carlos Crespo <[email protected]>
x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.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.
ftr_configs.yml
) => { | ||
if (!publicBaseUrl || !alertsLocator || !alertUuid) return ''; | ||
|
||
const rangeFrom = moment(startedAt).subtract('5', 'minute').toISOString(); |
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.
We probably want to take the window / look-back into account. Or perhaps rule interval. And consider multiples of those - for example, the range should be 3 x window, or something. If the look-back was an hour, or a day, seeing 5m may not be useful.
We'll probably get additional requirements for this anyway, so deferring till then is probably fine.
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 point, I was also thinking about the same but wanted to have a smaller PR focused on generating the link for infra rules for now.
In another instance, I used 1/5 of the alert duration period before the start of the alert and 1/5 after the alert ended (in case it was recovered.)
There is room for improvement, and I created the following ticket to improve this time range:
#158480
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.
Had a brief discussion on Slack that we should add some .github/CODEOWNERS
magic for the newly added x-pack/test/api_integration/apis/metrics_ui
directory, so that PR flagging for these go to the right team.
Other than that LGTM, since there weren't any other ResponseOps changes ...
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
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Closes #156534
Summary
This PR adds the alertDetailsUrl to the infra rules. The value of this variable is a link to the
observability > alerts
page filtered for this instance of alert.Here is an example of this action variable:
Note
kibana.alert.url
in another ticket🧪 How to test
server.publicBaseUrl
is configured in kibana.dev.ymlcontext.alertDetailsUrl
in action for this rule