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.

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

Ability to filter alerts by string parameters #92036

Merged
merged 5 commits into from
Feb 22, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ const mockMappings = {
},
},
},
params: {
type: 'flattened',
},
},
},
hiddenType: {
Expand Down Expand Up @@ -168,6 +171,12 @@ describe('Filter Utils', () => {
).toEqual(esKuery.fromKueryExpression('alert.actions:{ actionTypeId: ".server-log" }'));
});

test('Assemble filter for flattened fields', () => {
expect(
validateConvertFilterToKueryNode(['alert'], 'alert.attributes.params.foo:bar', mockMappings)
).toEqual(esKuery.fromKueryExpression('alert.params.foo:bar'));
});

test('Lets make sure that we are throwing an exception if we get an error', () => {
expect(() => {
validateConvertFilterToKueryNode(
Expand Down
36 changes: 23 additions & 13 deletions src/core/server/saved_objects/service/lib/filter_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,23 +207,33 @@ export const hasFilterKeyError = (

export const fieldDefined = (indexMappings: IndexMapping, key: string): boolean => {
const mappingKey = 'properties.' + key.split('.').join('.properties.');
const potentialKey = get(indexMappings, mappingKey);
if (get(indexMappings, mappingKey) != null) {
return true;
}

// If the `mappingKey` does not match a valid path, before returning null,
// If the `mappingKey` does not match a valid path, before returning false,
// we want to check and see if the intended path was for a multi-field
// such as `x.attributes.field.text` where `field` is mapped to both text
// and keyword
if (potentialKey == null) {
const propertiesAttribute = 'properties';
const indexOfLastProperties = mappingKey.lastIndexOf(propertiesAttribute);
const fieldMapping = mappingKey.substr(0, indexOfLastProperties);
const fieldType = mappingKey.substr(
mappingKey.lastIndexOf(propertiesAttribute) + `${propertiesAttribute}.`.length
);
const mapping = `${fieldMapping}fields.${fieldType}`;

return get(indexMappings, mapping) != null;
} else {
const propertiesAttribute = 'properties';
const indexOfLastProperties = mappingKey.lastIndexOf(propertiesAttribute);
const fieldMapping = mappingKey.substr(0, indexOfLastProperties);
const fieldType = mappingKey.substr(
mappingKey.lastIndexOf(propertiesAttribute) + `${propertiesAttribute}.`.length
);
const mapping = `${fieldMapping}fields.${fieldType}`;
if (get(indexMappings, mapping) != null) {
return true;
}

// If the path is for a flattned type field, we'll assume the mappings are defined.
const keys = key.split('.');
for (let i = 0; i < keys.length; i++) {
const path = `properties.${keys.slice(0, i + 1).join('.properties.')}`;
if (get(indexMappings, path)?.type === 'flattened') {
return true;
}
}
Comment on lines +230 to +236
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/kibana-core this PR is ready for review. I had to add this piece of code and a few early returns to support flattened type fields in alerting's use case.


return false;
};
3 changes: 1 addition & 2 deletions x-pack/plugins/alerts/server/saved_objects/mappings.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@
}
},
"params": {
"enabled": false,
"type": "object"
"type": "flattened"
},
"scheduledTaskId": {
"type": "keyword"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ export default function createFindTests({ getService }: FtrProviderContext) {

afterEach(() => objectRemover.removeAll());

async function createAlert(overwrites = {}) {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData(overwrites))
.expect(200);
objectRemover.add(Spaces.space1.id, createdAlert.id, 'alert', 'alerts');
return createdAlert;
}

it('should handle find alert request appropriately', async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert`)
Expand Down Expand Up @@ -85,5 +95,23 @@ export default function createFindTests({ getService }: FtrProviderContext) {
data: [],
});
});

it('should filter on string parameters', async () => {
await Promise.all([
createAlert({ params: { strValue: 'my a' } }),
createAlert({ params: { strValue: 'my b' } }),
createAlert({ params: { strValue: 'my c' } }),
]);

const response = await supertest.get(
`${getUrlPrefix(
Spaces.space1.id
)}/api/alerts/_find?filter=alert.attributes.params.strValue:"my b"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's feeling like we're leaking Alerting internal implementation here...

I'm ok with that, as it feels like the overhead of trying to abstract the attributes away is probably not worth it.... but I. would like this to have explicit documentation added (asciidocs/dev docs) as I don't think it's intuitive :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmmorris Agreed, I always felt like that with the KQL filter. Though, the enforcement of attributes is made by the saved objects service so I figured we'd pass it through as we do with other fields.

Under filter docs (https://www.elastic.co/guide/en/kibana/master/alerts-api-find.html) the params would work the same as any other field but I think it makes sense to document that params are analyzed as flattened and point to those docs so users understand the limitations of these fields.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, makes sense.
Putting myself in the mindset of a developer using our APIs - if the API explains what params is and leads me to an API that explains how to query against it - that's fine.

Do we actually explain that the Alerts are stored as SOs? 🤔
I'm not sure the SO Api makes it clear how to query attributes though 🤔 I'll take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the docs:

(Optional, string) A KQL string that you filter with an attribute from your saved object. It should look like savedObjectType.attributes.title: “myTitle”. However, If you used a direct attribute of a saved object, such as updatedAt, you will have to define your filter, for example, savedObjectType.updatedAt > 2018-12-22.

I think the only extra mention we need is that they're flattened and have filtering limitations.

);

expect(response.status).to.eql(200);
expect(response.body.total).to.equal(1);
expect(response.body.data[0].params.strValue).to.eql('my b');
});
});
}