[Logs UI] [Alerting] "Group by" functionality

[Kerry350]

Summary

This PR implements #67465. (UI and server side executor logic).

The design differs from the orignal ticket. It was agreed after a discussion that we should make the group by a part of the expression like everything else.

This expression piece has been implemented so it can be used within Metrics too. Currently this is in components/alerting/shared/group_by_expression, but I'm completely open to moving this if we're unhappy.

Note on tests: I've amended the current executor tests so that a green build is possible. However, now that there's an ungrouped logic fork and a group by logic fork, I'd like to A) quite heavily amend the test logic and B) add group by coverage. I'd like to do this in a followup PR directly after this work. < #69261

Functionality overview

Overall view of the group by expression:

Server logs for grouped and ungrouped alerts:

Ungrouped (closed):

Ungrouped (open):

Grouped (open):

Action message with new context.group:

Known issues

The combo box list is currently rendering under the popover due to a z-index issue, however this isn't easy to fix. This particular combination of flyout, popover, and combobox causes problems. I'm talking with the EUI team about this, but it shouldn't block review. < Fixed in elastic/eui#3551

Testing

SSL needs to be enabled: https://github.com/elastic/observability-dev/issues/849#issuecomment-626770782

• Does creating an alert through the logs app work with a group by?
• Does creating an alert through the logs app work without a group by?
• Does creating an alert through the alerts management page work with a group by?
• Does creating an alert through the alerts management page work without a group by?
• Does editing an alert through the alerts management page work with a group by?
• Does editing an alert through the alerts management page work without a group by?
• General usage

[katrin-freihofner]

🙌this looks great! Special thanks for the great documentation in the PR itself.

[elasticmachine]

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

[weltenwort]

👀 Reviewing on behalf of @elastic/logs-metrics-ui...

[weltenwort]

@elasticmachine merge upstream

[weltenwort]

This is the first batch of comments - I'm almost done but will have to defer the rest until tomorrow. It's pretty readable so far 👏

[weltenwort]

Currently documents that lack any of the grouping fields are not alerted on, i.e. there's no "everything else" bucket. Is that intentionally left out?

[Kerry350]

Yep. It was never explicitly discussed, however, so it is just a decision I've made. In my opinion if I've requested to group by host.name as an example, due to a big fleet of infrastructure, I don't think knowing something matched on "everything else" is helpful. However, I appreciate there's many fields, and many uses for grouping. Setting the group bys is an explicit action though, it would seem odd to me to get an alert on other things. Willing to throw this out to a wider audience for opinons though.

[weltenwort]

Yes, it really depends on the use case. Maybe @mukeshelastic can weigh in on that?

[weltenwort]

It seems to work in most cases, but I left some questions about edge cases below. They might just be due to a lack of context on my side, though. 😇

[weltenwort]

Another question that came to mind: Do you know why we use all of must, must_not and filter in the bool clause of the alert query? Since we're not interested in the scores just using must_not and filter should improve caching and thereby performance for these regularly-repeated queries.

[Kerry350]

Another question that came to mind: Do you know why we use all of must, must_not and filter in the bool clause of the alert query? Since we're not interested in the scores just using must_not and filter should improve caching and thereby performance for these regularly-repeated queries.

Just poor querying on my part. I didn't know a performance hit would be incurred for using must as well.

[Kerry350]

Yes, one way in which we could further soften the expectation without completely changing the way we query the data would be to use different (larger) time range in which we evaluate the existence of a group.

👍

Alternatively the groups could be captured statically at alert creation time so there's no risk of losing them, but that almost sounds like a completely different definition of "grouping".

Personally I'd strongly argue against this. With alerting I feel there's a good expectation that alerts are a reflection of execution time, and not "stale" data. Also (and I may well be missing something) I don't see how this would help — even if we captured the groups at creation time, later when the executor runs if there were "no documents" we'd still be in the same situation trying to ascertain which groups produced the "no documents", i.e. we wouldn't be able to link the statically captured groups to the runtime documents, as the "group by" fields would still be missing.

⚠️ I think this is good to go now, in terms of ES query amendments this is a summary of the changes:

• I've refactored the code a little so that grouped and ungrouped ES queries are now constructued in separate functions.

• For grouped scenarios the composite aggregation happens top-level, with the filtering as a nested sub-aggregation.

• For grouped scenarios the range receives a 20% (no science here, just tried to use something that wouldn't be so large it produced vastly different behaviour, but enough to potentially capture groups that slip into a no document state) padding (of the overall interval) on each side (gte and lte). This allows us to cast a slightly wider net for the groups themselves, incase the inner filtering "loses" the groups.

• There is now much more aggressive runtime type validation via io-ts. The alert params are validated. And so are ES search responses.

Edit: Forgot to push one change, now should be good.

[weltenwort]

Thanks for refactoring to make it even more readable - I think it's almost good to go 👏 And the type guards seem like a good thing too 👍

Your solution to the "disappearing groups" situation makes sense to me. Now it'll likely alert once when a group disappears. That's probably good enough for most cases. And as you said, the general "no data" scenario should probably be handled separately anyway.

One of my comments from last time about the fields still being limited to string seems to have fallen through the cracks. 😉

[weltenwort]

Any idea about this? 😇

[Kerry350]

@weltenwort

Any idea about this? 😇

It won't let me reply directly to this inline for some reason, so I'll do it here.

Sorry, I wasn't ignoring this, I just didn't have a good answer at the time and needed to go away and actually find out why 😬

So, right back at the beginning this filtering criteria was copied from metrics. And I didn't investigate it.

I wasn't easily able to find out precisely what dictated a field being searchable (maybe there' some docs I just missed), but I did find a Slack conversation that explained it.

A field is deemed as searchable when the index field mapping is set to true, and is therefore "queryable". Discover only reveals fields that aren't searchable when an option is toggled for "hidden" fields.

So I guess the question is, do we want people to be able to select fields that aren't set to be indexed and queryable? (Naively I'd say no?).

(I also changed the group by fields to allow only aggregatable fields).

[weltenwort]

So I guess the question is, do we want people to be able to select fields that aren't set to be indexed and queryable? (Naively I'd say no?).

(I also changed the group by fields to allow only aggregatable fields).

My understanding is that searchable indicates that an inverted index exists, so a lookup from term to documents that contains it is possible. aggregatable indicates that "doc values" exists such that an efficient lookup of terms that a field in a document contains is possible.

For the purposes of grouping we only require it to be aggregatable, I think.

[Kerry350]

My understanding is that searchable indicates that an inverted index exists, so a lookup from term to documents that contains it is possible. aggregatable indicates that "doc values" exists such that an efficient lookup of terms that a field in a document contains is possible.

Ah, okay, thanks for the explanation 👍 I'm struggling to find where this information exists (I trust you, you have way more experience with this, I just don't know where I'd point someone to find out about this).

I've tweaked the filtering now, so everything should be aligned with what's necessary.

This is all ready for another look over, thanks for the review patience 🙏

[weltenwort]

I'm struggling to find where this information exists

I was unable to find direct documentation of it either. The best I could find are the comments in elastic/elasticsearch#23007 and the ES source:

    /**
     * Returns true if the field is searchable.
     */
    public boolean isSearchable() {
        return isIndexed;
    }

    /** Returns true if the field is aggregatable. [sic]
     *
     */
    public boolean isAggregatable() {
        try {
            fielddataBuilder("");
            return true;
        } catch (IllegalArgumentException e) {
            return false;
        }
    }

[weltenwort]

I didn't come across any unexpected behavior anymore and the code looks better to me than before this PR. Good job 👏

(one final attempt to rectify my confusion in the comments)

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 17b49f8

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
infra	607	+1	606

History

• 💚 Build #57056 succeeded a6dfeb6
• 💚 Build #54336 succeeded 8cae6b6
• 💔 Build #53959 failed 8093dca
• 💔 Build #53696 failed 16608ee
• 💔 Build #53691 failed c9e56a2

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