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

[Lens] Enable include/exclude in terms aggreation #96585

Closed
timroes opened this issue Apr 8, 2021 · 6 comments · Fixed by #136179
Closed

[Lens] Enable include/exclude in terms aggreation #96585

timroes opened this issue Apr 8, 2021 · 6 comments · Fixed by #136179
Assignees
Labels
enhancement New value added to drive a business result Feature:Lens needs design Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@timroes
Copy link
Contributor

timroes commented Apr 8, 2021

The terms aggregation supports an include/exclude parameter. This allows filtering out values or including them that are shown in the top x values.

How is it different from using a filter to filter out? A filter will filter out the whole document a value appears. Since every field can have multiple values, users would like to still include the document, just filter out a specific value. Common use-cases are fields containing labels or tags, and you'd like to see the top x labels, but exclude some specific ones (e.g. see https://discuss.elastic.co/t/field-contains-csv-want-to-extract-to-array-of-strings/269494). Other real world use-case: indexing all GitHub issues and just wanting to see the top Team labels, thus you only want top terms matching Team:*.

In the visualize editor we expose the raw regex you can send as a text box. I don't think that's the right behavior for Lens. I think we should think about a UI, where the user can explicitly add a list of terms. I think the most use-case for adding a regex ist to allow for sinmple wildcard, like above mentioned Team:*. I wonder if we could build that simple wildcard mechanism (and compile it to a regexp) instead of exposing the full regexp. Meaning I'd suggest the user can add a list of terms for include and exclude, that can include a simple wildcard symbol.

@timroes timroes added enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Apr 8, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

UI: Two combo boxes in the advanced settings of top values, free input allowed with options sourced from the existing terms from activeData

@stratoula stratoula self-assigned this Jul 11, 2022
@stratoula
Copy link
Contributor

I am taking it. An important note here after a short investigation.

The include and exclude args on the terms expression are wrong: They should be multi: true (accept array of strings) https://github.com/elastic/kibana/blob/main/src/plugins/data/common/search/aggs/buckets/terms_fn.ts#L125
OR a string for patterns (for example: "ES-A.*")

Check here include/exclude can accept either a pattern or an array of strings/numbers https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-terms-aggregation.html

But there is another bug. This won't work if the terms are strings because here https://github.com/elastic/kibana/blob/main/src/plugins/data/common/search/aggs/buckets/migrate_include_exclude_format.ts#L38

we don't take under consideration the Array.isArray(value) && value.length > 0 && isStringType(aggConfig).

The function should change as:

    if (Array.isArray(value) && value.length > 0 && isNumberType(aggConfig)) {
      const parsedValue = value.filter((val): val is number => Number.isFinite(val));
      if (parsedValue.length) {
        output.params[this.name] = parsedValue;
      }
    } else if (Array.isArray(value) && value.length > 0 && isStringType(aggConfig)) {
      output.params[this.name] = value;
    } else if (isObject(value)) {
      output.params[this.name] = (value as any).pattern;
    } else if (value && isStringType(aggConfig)) {
      output.params[this.name] = value;
    }

cc @flash1293

@flash1293
Copy link
Contributor

flash1293 commented Jul 11, 2022

Thanks for the detective work @stratoula - I think we can adjust the logic like you defined. I guess it didn't come up yet because you can only specify a single string in the agg based interface. For our planned UI however it would make sense to allow lists, too

@stratoula
Copy link
Contributor

The only thing that troubles me is that on the expression function every time I define 'multi: true', the property becomes of Array type and I can't have something like string | string [].

@flash1293
Copy link
Contributor

Yeah, it will be an array with one item in this case. But existing consumers should be easy to adjust

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens needs design Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants