[Metrics UI] Fix alerting when a filter query is present #64575
Conversation
Zacqary
added
release_note:fix
Feature:Alerting
Feature:Metrics UI
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
|
Should we mark this as a Notable Fix for 7.7.1? The Filter bar in metrics alerts was completely broken, but I'm not sure if that fits our rigid blocker criteria? |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
This looks good to me and works! I do think this should be merged into 7.7. @simianhacker thoughts?
| return [ | ||
| { | ||
| query_string: { | ||
| query: filterQuery, |
There was a problem hiding this comment.
At this point isn't this a Kuery filter? We should be using convertKueryToElasticSearchQuery in x-pack/plugins/infra/public/utils/kuery.ts to convert it.
There was a problem hiding this comment.
@simianhacker We discussed this, but convertKueryToElasticSearchQuery relies on UI code. In another PR, I run convertKueryToElasticSearchQuery on all of the filter queries on the frontend.
https://github.com/elastic/kibana/pull/64292/files#diff-4ff0f0b3e6bb8caf3698ebd4b6373bfc
There was a problem hiding this comment.
If you look at that function you will see that it internally it links to some frontend code. BUT the same underlying functions are available on in the server part of the data plugin.
There was a problem hiding this comment.
So would it be redundant to add convertKueryToElasticSearchQuery on the frontend if we add it to the backend? Or vice versa? I'm wondering if the backend should just expect one or the other and not have a case handler for both of them.
The reason this bug happened is because the frontend used to send converted ES queries (which worked) and then at some point changed to KQL (which, as it turned out, did not work, but failed silently). So maybe we should just pick one and throw an error if we receive the wrong one?
There was a problem hiding this comment.
It shouldn't be a problem to always do it on the frontend 🤷♀️from what I can tell. That change is a bit more intrusive though. We basically need to start storing two fields text and query. Query is what the backend would use and text is what the frontend would display.
There was a problem hiding this comment.
Also, we need to do it on the frontend for validation at least, so that might be another argument for doing it on the frontend.
There was a problem hiding this comment.
@simianhacker How about we merge this as-is just to get filter queries working again, and then remove backend KQL support after merging #64292? That way the backend will only expect a converted query and error out if it's some other arbitrary string?
There was a problem hiding this comment.
Doing it on the front-end is sufficient, that's what we are doing on Metrics Explorer. I actually prefer the API's to be unaware of "Source", "Kuery", or whatever thing requires some special service to convert it into something ES can understand.
There was a problem hiding this comment.
Going to merge this just so we don't have broken functionality on master and track removing KQL from the backend in #64832
* master: (42 commits) [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452) [Endpoint] Remove todos, urls to issues (elastic#64833) [Uptime] Remove hard coded value for monitor states histograms (elastic#64396) Feature/send feedback link (elastic#64845) [ML] Moving get filters capability to admin (elastic#64879) Remove edit alert button from alerts list (elastic#64643) [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876) [ML] Disable data frame anaylics clone button based on permission (elastic#64830) Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767) add generic typings for SavedObjectMigrationFn (elastic#63943) Allow to define and update a defaultPath for applications (elastic#64498) [Event Log] add rel=primary to saved objects for query targets (elastic#64615) [Lens] Use a size of 5 for first string field in visualization (elastic#64726) [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) ...
…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (90 commits) remove unused import address review feedback [Ingest pipelines] Cleanup (elastic#64794) [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) skip flaky suite (elastic#64812) (elastic#64723) [Maps] do not display EMS or kibana layer wizards when not configured (elastic#64554) [Reporting/Test] Convert functional test code to Typescript (elastic#64601) make inserting timestamp with navigate methods optional with default true (elastic#64655) [EPM] Update UI to handle package versions and updates (elastic#64689) Minimize dependencies required by our telemetry middleware (elastic#64665) [Telemetry] oss api tests (elastic#64602) [ML] Adding endpoint capability checks (elastic#64662) Update jest config for coverage (elastic#64648) [SIEM][NP] Fixes bug in ML signals promotion (elastic#64720) share single data plugin bundle (elastic#64549) ...
Summary
Fixes #64538
Alerts containing a filter query previously would not fire, ever, even if it were supposed to, because of a mistake in the way the Elasticsearch query was constructed. This PR fixes that mistake and moves the parsed filter query into the correct spot.