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] Allow filtering on metric vis #131601

Merged
merged 13 commits into from
May 10, 2022
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented May 5, 2022

Fixes #122879

Makes it possible to filter a single metric visualization:

Filters by "exists" on the selected field:

Kapture.2022-05-06.at.16.18.51.mp4

If the metric is a filtered metric ("filter by" advanced option), it will set that filter instead of exists:

Kapture.2022-05-06.at.16.19.45.mp4

This makes it possible to attach drilldowns to metric visualizations.

Considerations

  • Count metric can't be meaningfully filtered (it just doesn't create a filter at the moment)
  • Filtering is also available for agg based metrics (as they are using the same renderer under the hood)

@flash1293 flash1293 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens backport:skip This commit does not require backporting v8.3.0 and removed skip-ci labels May 6, 2022
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 marked this pull request as ready for review May 7, 2022 10:09
@flash1293 flash1293 requested review from a team as code owners May 7, 2022 10:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@mbondyra
Copy link
Contributor

mbondyra commented May 9, 2022

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Overall it's a nice functionality, but I feel like we should maybe add something to indicate what will happen when you click. User gets no message and it's sometimes tricky to see the filter appeared on the dashboard. Sometimes you might not even see the relationship between the click and the filter because the label doesn't match the field name. Even showing a little browser tooltip with 'click to filter' would feel better for me. Or (maybe too invasive though) showing some subtle notification 'filter successfully created!' 😀

May-09-2022.11-15-05.mp4

@ghudgins WDYT?

Apart from it, if we decide to go this route, a small nit:

Count metric can't be meaningfully filtered (it just doesn't create a filter at the moment)

Could we not change the cursor to the pointer when user hovers over the metric where there's no filter to create?

@flash1293
Copy link
Contributor Author

Could we not change the cursor to the pointer when user hovers over the metric where there's no filter to create?

yeah, good point, I’m going to build that

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app services changes LGTM

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

@mbondyra

Could we not change the cursor to the pointer when user hovers over the metric where there's no filter to create

I tried to implement this but due to the way filters are wired up right now this is very difficult - it would require changes pretty deep within the filter handling of the data plugin. Due to that I would like to split out this part, what do you think?

Even showing a little browser tooltip with 'click to filter' would feel better for me

This makes sense, adjusted like this:
Screenshot 2022-05-10 at 10 22 08

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 506 507 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2857 2861 +4
expressionMetricVis 46 48 +2
total +6

Async chunks

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

id before after diff
expressionMetricVis 11.1KB 11.3KB +178.0B
expressionTagcloud 7.7KB 7.7KB -6.0B
lens 1.1MB 1.1MB +41.0B
visTypeTable 15.8KB 15.8KB -6.0B
visTypeTimeseries 465.3KB 465.3KB -6.0B
visTypeVislib 349.0KB 349.0KB -12.0B
total +189.0B

Page load bundle

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

id before after diff
charts 46.2KB 46.2KB -12.0B
data 413.8KB 414.2KB +369.0B
total +357.0B
Unknown metric groups

API count

id before after diff
data 3469 3473 +4
expressionMetricVis 46 48 +2
total +6

History

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

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Approved, thank you for looking into my comments - this is a great improvement :)

Due to that I would like to split out this part, what do you think?

Definietely, it's a small thing and doesn't happen a lot (probably just for count or formulas?) so I am ok with deprioritizing it. :) Adding to that, maybe you could also add aria with a message select to filter when the element is filterable? But that's a similar problem so totally can be left out for now.

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:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Allow metric visualization to drill down
6 participants