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 treemap in suggestions #169095

Merged
merged 7 commits into from
Jan 11, 2024

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Oct 17, 2023

Summary

Enable treemaps in suggestions outside the partition chart realm.

Checklist

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v8.12.0 labels Oct 17, 2023
@dej611 dej611 marked this pull request as ready for review October 18, 2023 10:58
@dej611 dej611 requested a review from a team as a code owner October 18, 2023 10:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@stratoula
Copy link
Contributor

@dej611 thank you!

If you go to Discover, ES|QL mode and run a query like:

from logstash-* | keep bytes, clientip, geo.dest

I expect to see treemap in the suggestions and I see it. The problem is that when I select I get an error and is never rendered

treemap

It seems as the problem is on EC? Can you investigate and possibly create an issue? If treemap has performance issues in text based mode possibly we should not depict it there for now.

@stratoula
Copy link
Contributor

You can also hit in dataview mode
image

while the mosaic seems to render ok. 🤔

cc @nickofthyme

@dej611
Copy link
Contributor Author

dej611 commented Oct 18, 2023

You can also hit in dataview mode image

while the mosaic seems to render ok. 🤔

cc @nickofthyme

I can reproduce the issue in main as well, it seems a bug on the EC side.
Also I've noticed also that this happens with values in the order of the thousands: i.e. Count or Unique count seems not affected by that, but Average(bytes) or Sum(bytes) have problems. Perhaps the logic to workout how much space to allocate for each bucket is the problem cc @nickofthyme ?

Edit: the bug was already logged in our issue tracker here some time ago => #164730

@stratoula
Copy link
Contributor

@dej611 so we have already an issue! Nice. So I just wonder now, does it make sense to proceed with this PR? It is very easy to hit it especially on ES|QL mode

@dej611
Copy link
Contributor Author

dej611 commented Oct 18, 2023

@dej611 so we have already an issue! Nice. So I just wonder now, does it make sense to proceed with this PR? It is very easy to hit it especially on ES|QL mode

I'll check the EC codebase and see whether a quick fix is viable there to unblock this and fix the logged issue

@dej611
Copy link
Contributor Author

dej611 commented Dec 19, 2023

/ci

@dej611
Copy link
Contributor Author

dej611 commented Dec 19, 2023

I think since #170914 got merged with the fix for treemap we could reconsider this PR again.

@stratoula
Copy link
Contributor

@dej611 I still hit the bug 🤔 (I have bootstrapped)

image

@dej611
Copy link
Contributor Author

dej611 commented Dec 20, 2023

@dej611 I still hit the bug 🤔 (I have bootstrapped)

image

Yeah, I thought it was fixed but @markov00 confirmed that the fix has not been merged into Kibana yet. Will be available after EC 61.1.0

@markov00
Copy link
Member

the fix will be in Kibana when #173329 will be merged

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #46 / aiops log pattern analysis loads the log pattern analysis page and filters in patterns in discover

Metrics [docs]

Async chunks

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

id before after diff
lens 1.4MB 1.4MB -61.0B

History

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

@stratoula
Copy link
Contributor

It works now 🎸 🥳 🎉 Thanx @markov00 and @dej611

image

Copy link
Contributor

@stratoula stratoula 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 added v8.13.0 and removed v8.12.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 11, 2024
@dej611 dej611 merged commit cc2da71 into elastic:main Jan 11, 2024
23 of 24 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 11, 2024
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
## Summary

Enable treemaps in suggestions outside the partition chart realm.


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
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.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants