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

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Feb 19, 2021

Resolves #92010

In this PR, I'm allowing alert parameters to be filtered on string values.

@mikecote mikecote added release_note:enhancement Feature:Alerting v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 19, 2021
@mikecote mikecote self-assigned this Feb 19, 2021
@mikecote mikecote marked this pull request as ready for review February 19, 2021 19:06
@mikecote mikecote requested review from a team as code owners February 19, 2021 19:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Comment on lines +230 to +236
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;
}
}
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.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm curious now if this can solve our "internal tags" requirement. I think it will require the per-type alert UIs to pre-fill these some new "hidden" params (hidden from the UI). For some reason I didn't realize you could access the individual fields in a flattened type in a query, I thought it just jammed all the text for all keys into a single field (which makes me curious how it's implemented)!

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.

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, I'd just like this to add some docs as I don't think using this is intuitive considering it relies on Alerting/SOs internal implementation

@gmmorris
Copy link
Contributor

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@pmuellr

I'm curious now if this can solve our "internal tags" requirement. I think it will require the per-type alert UIs to pre-fill these some new "hidden" params (hidden from the UI). For some reason I didn't realize you could access the individual fields in a flattened type in a query, I thought it just jammed all the text for all keys into a single field (which makes me curious how it's implemented)!

Yeah, it works pretty well which is why I didn't understand why there would be a use case for "internal tags" if we had this. I think we'll be able to close the issue in favour of this and ask for use-cases if ever this doesn't solve it. (one less thing to maintain).

@@ -49,6 +49,8 @@ Retrieve a paginated set of alerts based on condition.
NOTE: As alerts change in {kib}, the results on each page of the response also
change. Use the find API for traditional paginated results, but avoid using it to export large amounts of data.

NOTE: Alert `params` are stored as {ref}/flattened.html[flattened] and analyzed as `keyword`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gchaps let me know what you think, I wanted to state that filtering on the params field has limitations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've merged this PR, but I will create a follow-up if you would like changes to this line.

@mikecote mikecote requested a review from gchaps February 22, 2021 13:56
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
alert 32 33 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mikecote

@mikecote mikecote merged commit 0c2495a into elastic:master Feb 22, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Feb 22, 2021
* Initial commit

* Update comment

* Return early

* Add docs

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #92219

Successful backport PRs will be merged automatically after passing CI.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 22, 2021
* master:
  Ability to filter alerts by string parameters (elastic#92036)
  [APM] Fix for flaky correlations API test (elastic#91673) (elastic#92094)
  [Enterprise Search] Migrate shared role mapping components (elastic#91723)
  [file_upload] move ml Importer classes to file_upload plugin (elastic#91559)
  [Discover] Always show the "hide missing fields" toggle (elastic#91889)
  v2 migrations should exit process on corrupt saved object document (elastic#91465)
  [ML] Data Frame Analytics exploration page: filters improvements (elastic#91748)
  [ML] Data Frame Analytics: Improved error handling for scatterplot matrix. (elastic#91993)
  [coverage] speed up merging results of functional tests (elastic#92111)
  Adds a Reason indicator to the onClose handler in AddAlert and EditAlert (elastic#92149)
kibanamachine added a commit that referenced this pull request Feb 22, 2021
* Initial commit

* Update comment

* Return early

* Add docs

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Mike Côté <mikecote@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to filter alerts by their string parameters
6 participants