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][Agg based table]Navigate to lens for agg based table vis #140791

Merged
merged 264 commits into from
Sep 26, 2022

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Sep 15, 2022

Summary

Completes part of #138236.

As part of phasing out TSVB and Visualize all Legacy agg based visulizations should support "open in lens" functionality.
In that PR converter for Agg based table was added.

@Kuznietsov
Copy link
Contributor

Kuznietsov commented Sep 22, 2022

  1. The operations that allow dates (or geofield) in datatable but not in Lens cause improper dimensions created. I think we should either remove them or not allow to convert to lens when they occur. Here’s the 3 examples:
Screenshot 2022-09-22 at 11 13 13

Done.

@Kuznietsov
Copy link
Contributor

@elasticmachine merge upstream

@spalger
Copy link
Contributor

spalger commented Sep 22, 2022

@elasticmachine merge upstream

(to resolve ci issues caused by Github outage)

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.

There are two small inconsistencies that can be target later if at all:

  1. The histogram is not giving the same intervals as converted intervals:

Screenshot 2022-09-23 at 12 09 20

Screenshot 2022-09-23 at 12 09 25

  1. For the field types, it doesn't allow to convert for simple metric if used the not suppored fieldtype, like this one:

Screenshot 2022-09-23 at 12 00 58

But for cumulative_sum of the same metric it allows to convert and then shows nice readable error message:
Screenshot 2022-09-23 at 12 01 15
converts to:
Screenshot 2022-09-23 at 12 01 19

I find neither of them critical so approving. It can be addressed later.

@VladLasitsa
Copy link
Contributor Author

There are two small inconsistencies that can be target later if at all:

  1. The histogram is not giving the same intervals as converted intervals:
Screenshot 2022-09-23 at 12 09 20 Screenshot 2022-09-23 at 12 09 25

@mbondyra About that: we don't have possibility to provide interval for "intervals" in Lens.
We have only these params:
Снимок экрана 2022-09-23 в 14 08 51
In Lens implementation we always provide "auto" as interval for histogram:

Снимок экрана 2022-09-23 в 14 10 41

After discussion with Joe we decided to ignore interval for histogram.

cc @flash1293

@Kuznietsov
Copy link
Contributor

  1. For the field types, it doesn't allow to convert for simple metric if used the not suppored fieldtype, like this one:
Screenshot 2022-09-23 at 12 00 58

But for cumulative_sum of the same metric it allows to convert and then shows nice readable error message: Screenshot 2022-09-23 at 12 01 15 converts to: Screenshot 2022-09-23 at 12 01 19

@mbondyra, we've fixed this problem.

@mbondyra
Copy link
Contributor

Rechecked, and re-approving!

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.

Sorry for being so annoying with the sizes but + 24KB are a lot :) Can we look into it?

image

@stratoula stratoula removed the WIP Work in progress label Sep 23, 2022
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.

LGTM

@stratoula stratoula dismissed their stale review September 26, 2022 05:59

The bundle size has been fixed, so I am dismissing my review

@Kuznietsov
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 905 906 +1
visTypeTable 35 42 +7
visualizations 318 352 +34
total +42

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 2429 2506 +77
visualizations 602 649 +47
total +124

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.2MB 1.2MB +567.0B
visTypeTimeseries 475.4KB 475.8KB +390.0B
visualizations 218.6KB 239.2KB +20.6KB
total +21.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
visualizations 14 18 +4

Page load bundle

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

id before after diff
data 431.8KB 432.2KB +400.0B
lens 34.9KB 35.4KB +466.0B
visTypeTable 15.6KB 19.4KB +3.8KB
visTypeTimeseries 19.1KB 19.2KB +25.0B
visualizations 49.1KB 51.2KB +2.2KB
total +6.8KB
Unknown metric groups

API count

id before after diff
data 3132 3209 +77
visualizations 631 679 +48
total +125

async chunk count

id before after diff
visualizations 13 14 +1

History

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

cc @VladLasitsa @Kunzetsov

@Kuznietsov Kuznietsov merged commit f576b1f into elastic:main Sep 26, 2022
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 Feature:Vis Editor Visualization editor issues impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants