-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Logs UI] Alerting #62806
[Logs UI] Alerting #62806
Conversation
…f route handler contexts (like the alert executor)
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@Zacqary I've added you as the reviewer for this, please let me know if that's not okay. The only big(ish) caveat is the alerts management screen, which will be addressed in a followup. |
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.
Overall looks really good, couple of UX suggestions though
x-pack/plugins/infra/public/components/alerting/logs/expression_editor/criterion.tsx
Show resolved
Hide resolved
}} | ||
/> | ||
) : ( | ||
<EuiFieldText |
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.
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.
Hmm, I think this could potentially mislead / frustrate people. I understand the intention, but I can also see people entering error
and then seeing "error"
and thinking "huh, I haven't added quotations".
Or, if there's a scenario where people actually want to have the quotations as part of the value they're matching, we either need to display double quotes which is messy, or not render them if the user has used quotes in the value. This again leads to a misleading situation.
I think it might be best to leave this one? Or defer to design for the next iteration?
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.
Okay we can leave this. Changing EQUALS
to IS
should be good enough.
export async function registerMetricThresholdAlertType(alertingPlugin: PluginSetupContract) { | ||
export async function registerMetricThresholdAlertType( | ||
alertingPlugin: PluginSetupContract, | ||
libs: InfraBackendLibs |
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.
libs
is unused in this alert type, and on my end it doesn't look like we need to add this arg to get it to pass type check. Am I missing something?
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 was mostly laziness to deal with:
registerFns.forEach(fn => {
fn(alertingPlugin, libs);
});
it was easier just to pass them both the libs
.
As the work Philip and I are doing has some overlap here, where we've both refactored the source configuration code and allowed access to libs
in executors, and Philip has refactored this forEach
line, I think it would be easier just to leave this for this merge. And deal with the small conflict that will arise in Philip's branch.
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.
// cc @phillipb
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.
Sounds good, we can do that.
@Zacqary Thanks for the review 👍 I've responded to your feedback, if you could take another look. |
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.
Looks good!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
First iteration of logs alerting
…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (152 commits) [Ingest pipelines] Simulate pipeline (elastic#64223) Ability to get scoped call cluster from alerting and action executors (elastic#64432) Add editApp and editPath to embeddable (elastic#64297) TSVB validation: Allow numeric values for axes (elastic#63553) [ML] Fixing optional plugin dependency types (elastic#64450) [Logs UI] Alerting (elastic#62806) [Endpoint] Show Policy Status on Host Details using Policy Response API (elastic#64116) [Maps] update LayerWizard previewLayer to take layerDescriptor instead of ISource (elastic#64461) Remove SO root property index signature (elastic#64434) [ML] Functional tests - stabilize job row details validations (elastic#64503) [Ingest] Add Global settings flyout (elastic#64276) Bump cypress dev-dependency from 4.2.0 to 4.4.1 (elastic#64408) Migrate saved object of type url to kibana platform (elastic#64043) [NP] Migrate ui capabilities (elastic#64185) Bump karma-mocha dev-dependency from 1.3.0 to 2.0.0 (elastic#64407) Migrate kql_telemetry saved object registration to Kibana platform (elastic#64149) Remove SO autocreateindex error and error page (elastic#64037) Fix issue with yarn.lock (elastic#64496) Bump @hapi/boom dependency from 7.4.2 to 7.4.11 (elastic#64433) Bump gonzales-pe dev-dependency from 4.2.4 to 4.3.0 (elastic#64401) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/shared_imports.ts
Summary
This PR implements #61493. It adds the "core" of the first iteration of logs alerting. The end-to-end flow when adding an alert via the logs app should work.
Followups
Due to the size of this PR, and also working in parellel with other PRs, the following is to come in the next PR:
Alert management screen changes: there are caveats to editing an alert on the alerts management screen (i.e. the things you have access to via context). This flow will be addressed next, and should be considered broken with the work here. In part this is so that [Logs UI] Reimplement log source configuration routes in plain HTTP+JSON #64021 can be merged first, which addresses one of the issues there without having to create a workaround.
Tests. I would like to add executor tests at least.
The
occur
text mentioned here (after speaking with Felix this will appear after the "when log entries" line).Add a proper loading state to show when the fields data is loading (easier with the new logs source config hook).
Testing
SSL is necessary, which can be facilitated with:
yarn es snapshot --ssl
yarn start --ssl
And adding
ssl.verification_mode: none
in the Filebeat config.