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

Creates ES|QL where filters for ordinal charts #184420

Merged
merged 22 commits into from
Jun 10, 2024

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 29, 2024

Summary

Closes #183425

meow

This PR enables the creation of where clause filters by clicking a chart in Discover and wherever the ES|QL editor exists. This means that this is not available in dashboards.

This is possible only for ordinal charts. For date fields is quite difficult to know the interval so I don't allow it for now. We already support brushing so time filtering is already available.

Checklist

@stratoula stratoula changed the title Create ES|QL where filters for ordinal charts Creates ES|QL where filters for ordinal charts Jun 3, 2024
@stratoula stratoula added release_note:enhancement backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana v8.15.0 Feature:Discover Discover Application labels Jun 3, 2024
@stratoula stratoula marked this pull request as ready for review June 3, 2024 10:33
@stratoula stratoula requested review from a team as code owners June 3, 2024 10:33
@stratoula stratoula added the Team:ESQL ES|QL related features in Kibana label Jun 3, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

kibana.jsonc changes LGTM

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Did a first review round.

Noticed some possible improvement over the appendWhereClauseToESQLQuery logic:

  • forced casting fro ip field types

    from kibana_sample_data_logs | stats count() by ip | where ip == "55.237.229.59"
    

    that is a valid ES|QL, but currently what it gets configured is this version:

    from kibana_sample_data_logs | stats count() by ip | where `ip`::string=="55.237.229.59"
    
  • why is it always quoting variable/fields? There's an AST function that can help here, identifying when a variable/field needs to be wrapped.

  • where the filter is triggered by the multi-series tooltip is does not work

pie_filtering
heatmap_filtering

  • is it a bit annoying that when the where clause is added to the query there's the probability that the visualization changes. I see how that would make "more sense" for some data changes, but perhaps the process should include a prompt to "suggest" to the user the new chart type while keeping the previous type?

tag_cloud_filtering

When in dashboard I've noticed that legend items have the filter popup active. Of course clicking on them doesn't do anything, as the filtering I understand applies only to Discover, right?
This is present also in main, so it's ok to not specifically fix this here.

Screenshot 2024-06-04 at 12 07 39

@stratoula
Copy link
Contributor Author

stratoula commented Jun 4, 2024

@dej611 thanx. My answers to your questions:

forced casting fro ip field types ...

When we created the function ip fields needed casting. I want to revisit this function but I am waiting some more cast improvements from the ES team. I would do this on a follow up PR, it is irrelevant with the current, as this functionality already exists in main

why is it always quoting variable/fields? There's an AST function that can help here, identifying when a variable/field needs to be wrapped.

Same as above!

is it a bit annoying that when the where clause is added to the query there's the probability that the visualization changes. I see how that would make "more sense" for some data changes, but perhaps the process should include a prompt to "suggest" to the user the new chart type while keeping the previous type?

We have already an issue for this #184631

When in dashboard I've noticed that legend items have the filter popup active. Of course clicking on them doesn't do anything, as the filtering I understand applies only to Discover, right?

True I will create an issue for that

where the filter is triggered by the multi-series tooltip is does not work

Thanx I will take a look in this PR

@stratoula
Copy link
Contributor Author

@stratoula
Copy link
Contributor Author

@dej611 good catch with the tooltip actions, I disabled it for this PR (I had already done it for XY charts but not for heatmap and partition charts)

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple of questions on some async code

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

I think I'm seeing another bug. I'm using this query:

from kibana_sample_data_logs
| stats total_bytes = sum(bytes) by agent

And in the Lens configuration, I'm using agent as the Breakdown field, and when I click on a bar in the chart, it doesn't filter and I get this error:

create_filters_from_value_click.ts:124 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'meta')
    at create_filters_from_value_click.ts:124:1
    at Array.filter (<anonymous>)
    at appendFilterToESQLQueryFromValueClickAction (create_filters_from_value_click.ts:122:1)

Screen recording:

Screen.Recording.2024-06-06.at.1.53.03.PM.mov

@stratoula
Copy link
Contributor Author

It is due to @dej611 's recommendation. It worked before. I will look into it, thanx

@stratoula
Copy link
Contributor Author

@lukasolson fixed

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Okay, I found another issue, but I think it's an existing bug that just shows up in this PR because clicking on a bar before this PR does nothing.

  1. Load sample web logs
  2. Switch to ES|QL and use the following query:
from kibana_sample_data_logs
| stats total_bytes = sum(bytes) by extension, bytes % 5
  1. Edit the Lens visualization and drag bytes % 5 to the horizontal axis (notice the x-axis title still says "extension" even after you changed it
  2. Click on a bar in the chart

Instead of filtering on bytes % 5, it filters on extension and you get the following error:

[esql] > Unexpected error from Elasticsearch: verification_exception - Found 1 problem
line 3:9: first argument of [`extension`==2] is [text] so second argument must also be [text] but was [integer]

Screen recording:

Untitled.mov

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

This is really awesome! It's great to see this all coming together. Just added a couple of minor nits below. Regarding the other bug, I'm not sure it's something we need to fix in this PR since it's probably an existing bug.

// ES|QL charts have a different way of applying filters,
// they are appending a where clause to the query
const queryString = appendFilterToESQLQueryFromValueClickAction(context.data);
await getStartServices().uiActions.getTrigger('UPDATE_ESQL_QUERY_TRIGGER').exec({
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we use a constant (and export it) for 'UPDATE_ESQL_QUERY_TRIGGER'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didnt do it is because if I export it from the text-based languages plugin (where the trigger is being registered) it will create circular dependencies with the data plugin. I could export it from the esql-utils package but it seems a bit unrelated to me. So I will leave it as it is for now and I will think about it more when another usage will be required.

@stratoula
Copy link
Contributor Author

@lukasolson thanx, I created a separate issue for the bug here #185636 because the root cause is different. If you remove the columns and add it by clicking the dimension button and filter you get the filter correctly (and the axis(, the drag and drop does something weird here.

meow

@stratoula
Copy link
Contributor Author

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 499 512 +13
textBasedLanguages 54 58 +4
total +17

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionHeatmap 26.5KB 26.6KB +100.0B
expressionPartitionVis 36.4KB 36.5KB +87.0B
expressionXY 128.7KB 128.7KB -5.0B
textBasedLanguages 166.8KB 167.3KB +523.0B
total +705.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 418.5KB 420.1KB +1.6KB
textBasedLanguages 5.6KB 6.7KB +1.1KB
total +2.7KB
Unknown metric groups

async chunk count

id before after diff
textBasedLanguages 2 3 +1

History

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

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

LGTM!

@stratoula stratoula merged commit e1c86f1 into elastic:main Jun 10, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application Feature:ES|QL ES|QL related features in Kibana release_note:enhancement Team:ESQL ES|QL related features in Kibana v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] [Discover] Clicking the chart should create a where clause
8 participants