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.

Sign up for GitHub

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

Jump to bottom

[Metrics UI] Fix alerting when a filter query is present #64575

Merged
merged 3 commits into from
Apr 29, 2020
Merged

[Metrics UI] Fix alerting when a filter query is present #64575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfaf980
[Metrics UI] Fix alerting when a filter query is present
Zacqary Apr 27, 2020
0c7aefa
Fix parsing empty query
Zacqary Apr 28, 2020
5e2f802
Handle KQL and parsed ES cases
Zacqary Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,19 @@ const getCurrentValueFromAggregations = (

const getParsedFilterQuery: (
filterQuery: string | undefined
) => Record<string, any> = filterQuery => {
) => Record<string, any> | Array<Record<string, any>> = filterQuery => {
if (!filterQuery) return {};
try {
return JSON.parse(filterQuery).bool;
} catch (e) {
return {
query_string: {
query: filterQuery,
analyze_wildcard: true,
return [
{
query_string: {
query: filterQuery,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@phillipb phillipb Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to merge this just so we don't have broken functionality on master and track removing KQL from the backend in #64832

analyze_wildcard: true,
},
},
};
];
}
};

Expand Down Expand Up @@ -159,8 +161,12 @@ export const getElasticsearchMetricQuery = (
return {
query: {
bool: {
filter: [...rangeFilters, ...metricFieldFilters],
...parsedFilterQuery,
filter: [
...rangeFilters,
...metricFieldFilters,
...(Array.isArray(parsedFilterQuery) ? parsedFilterQuery : []),
],
...(!Array.isArray(parsedFilterQuery) ? parsedFilterQuery : {}),
},
},
size: 0,
Expand Down Expand Up @@ -233,6 +239,7 @@ const getMetric: (
body: searchBody,
index,
});

return { '*': getCurrentValueFromAggregations(result.aggregations, aggType) };
} catch (e) {
return { '*': undefined }; // Trigger an Error state
Expand Down
18 changes: 18 additions & 0 deletions x-pack/test/api_integration/apis/infra/metrics_alerting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export default function({ getService }: FtrProviderContext) {
});
expect(result.error).to.not.be.ok();
expect(result.hits).to.be.ok();
expect(result.aggregations).to.be.ok();
});
}
it('should work with a filterQuery', async () => {
Expand All @@ -53,6 +54,21 @@ export default function({ getService }: FtrProviderContext) {
});
expect(result.error).to.not.be.ok();
expect(result.hits).to.be.ok();
expect(result.aggregations).to.be.ok();
});
it('should work with a filterQuery in KQL format', async () => {
const searchBody = getElasticsearchMetricQuery(
getSearchParams('avg'),
undefined,
'"agent.hostname":"foo"'
);
const result = await client.search({
index,
body: searchBody,
});
expect(result.error).to.not.be.ok();
expect(result.hits).to.be.ok();
expect(result.aggregations).to.be.ok();
});
});
describe('querying with a groupBy parameter', () => {
Expand All @@ -65,6 +81,7 @@ export default function({ getService }: FtrProviderContext) {
});
expect(result.error).to.not.be.ok();
expect(result.hits).to.be.ok();
expect(result.aggregations).to.be.ok();
});
}
it('should work with a filterQuery', async () => {
Expand All @@ -79,6 +96,7 @@ export default function({ getService }: FtrProviderContext) {
});
expect(result.error).to.not.be.ok();
expect(result.hits).to.be.ok();
expect(result.aggregations).to.be.ok();
});
});
});
Expand Down