Skip to content

[TSVB][Lens] Add "open in lens" functionality for Top N#138200

Merged
VladLasitsa merged 14 commits intoelastic:mainfrom
VladLasitsa:convert_top_n_in_lens
Aug 22, 2022
Merged

[TSVB][Lens] Add "open in lens" functionality for Top N#138200
VladLasitsa merged 14 commits intoelastic:mainfrom
VladLasitsa:convert_top_n_in_lens

Conversation

@VladLasitsa
Copy link
Copy Markdown
Contributor

@VladLasitsa VladLasitsa commented Aug 5, 2022 •
edited by Kuznietsov
Loading

Summary

Completes part of #138236.

As part of phasing out TSVB and Visualize all TSVB visulizations should support "open in lens" functionality.
In that PR converter for Top N was added.
Also was added support of the following stuff:

  • value count agg
  • standard deviation agg
  • last value mode

Top N in TSVB:
Снимок экрана 2022-08-05 в 15 57 43

In Lens:
Снимок экрана 2022-08-05 в 15 58 15

@VladLasitsa VladLasitsa self-assigned this Aug 5, 2022
@VladLasitsa VladLasitsa added Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes Feature:Lens backport:skip This PR does not require backporting v8.5.0 WIP Work in progress labels Aug 5, 2022
@Kuznietsov Kuznietsov mentioned this pull request Aug 8, 2022
Closed
28 tasks
@VladLasitsa VladLasitsa marked this pull request as ready for review August 9, 2022 09:01
@VladLasitsa VladLasitsa requested a review from a team as a code owner August 9, 2022 09:01
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 9, 2022

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

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 9, 2022

@elasticmachine merge upstream

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 10, 2022

@elasticmachine merge upstream

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Aug 10, 2022

Hey! Thanx for working on this. Some comments and questions from my side:

I wonder why don't we allow the transition for multiple layers? For example:
image

can be translated to
image

As long this is not allowed in TSVB, we should not allow the transition to Lens either
image

The last value mode should be converted to Lens with the Reduced time range setting. Am I right @flash1293 ?
image

There are some aggs that work on TSVB such as the cumulative sum (due to the date_histogram) but not in Lens. Should we allow this transition? cc @flash1293
image

In case of percentile ranks split by terms the custom color is not transferred in Lens. So for example this will transition to Lens with the default green color.
image

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Aug 10, 2022

I wonder why don't we allow the transition for multiple layers

Vlad and me discussed this - as the chart looks pretty weird due to the empty slots for the other series, we decided to not convert these for now. However, looking at it again I think it's worth doing the conversion even if it looks a little weird - better than no chart. @VladLasitsa could you change this? Sorry for the back and forth

The last value mode should be converted to Lens with the Reduced time range setting. Am I right

yes exactly! I didn't mention this in the doc. We can also split it out. I'll update the doc

There are some aggs that work on TSVB such as the cumulative sum (due to the date_histogram) but not in Lens. Should we allow this transition?

No, let's block these please.

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Aug 10, 2022

Thanx Joe! I had updated the doc some days ago if I recall correctly!

@VladLasitsa VladLasitsa requested a review from a team as a code owner August 12, 2022 12:04
@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 12, 2022

@elasticmachine merge upstream

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 12, 2022

@stratoula, Could you please review again?

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Aug 12, 2022

Thanx Vlad! Found one bug!
If I am on TopN last value and then go back to timeseries and click "Navigate to Lens" I see this error
image

This happens because we also transfer the last value mode which is wrong as the timeseries chart is not compatible with this.

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 16, 2022

@elasticmachine merge upstream

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 16, 2022

Thanx Vlad! Found one bug! If I am on TopN last value and then go back to timeseries and click "Navigate to Lens" I see this error image

This happens because we also transfer the last value mode which is wrong as the timeseries chart is not compatible with this.

Fixed

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Aug 17, 2022

Looks pretty good, just found one bug: "Last value" mode is not respected for formula-type dimensions (like standard deviation or filter ratio) - it seems like it's just ignored for these cases.

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 17, 2022

Looks pretty good, just found one bug: "Last value" mode is not respected for formula-type dimensions (like standard deviation or filter ratio) - it seems like it's just ignored for these cases.

@flash1293, Could you please re-test?

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Aug 18, 2022

Seems like filter ratio is not convertable anymore:
Screenshot 2022-08-18 at 11 21 20

Maybe the check for "missing field" went wrong?

It also still tries to set the reduced time range when on a timeseries chart:
Screenshot 2022-08-18 at 11 22 44
Screenshot 2022-08-18 at 11 22 47

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Aug 18, 2022 •
edited
Loading

As discussed in the weekly sync, parent pipeline aggregations should not be convertable for charts other than time series because they are executed along the time axis and there isn't a time axis in Lens:
Screenshot 2022-08-18 at 11 25 50
Screenshot 2022-08-18 at 11 25 16

This conversion is not equivalent, the overall_sum is doing something else in Lens than in TSVB

flash1293
flash1293 approved these changes Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, tested and all cases work as expected. I will follow up on the moving average problem

ppisljar
ppisljar approved these changes Aug 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@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

@VladLasitsa
Copy link
Copy Markdown
Contributor Author

VladLasitsa commented Aug 22, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Aug 22, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 361 363 +2

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 2428 2431 +3
visualizations 386 388 +2
total +5

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 +182.0B
visTypeTimeseries 453.0KB 459.8KB +6.8KB
visualizations 197.4KB 197.4KB +88.0B
total +7.1KB

Page load bundle

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

id before after diff
data 429.5KB 429.6KB +52.0B
visTypeTimeseries 18.7KB 18.9KB +178.0B
total +230.0B
Unknown metric groups

API count

id before after diff
data 3114 3117 +3
visualizations 414 416 +2
total +5

async chunk count

id before after diff
visTypeTimeseries 11 13 +2

History

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

cc @VladLasitsa

@VladLasitsa VladLasitsa merged commit 750de11 into elastic:main Aug 22, 2022
@VladLasitsa VladLasitsa mentioned this pull request Aug 29, 2022
Closed
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
@VladLasitsa @kibanamachine
@flash1293 @Mpdreamz
[TSVB][Lens] Add "open in lens" functionality for Top N (elastic#138200)
377b5dc 
* Create lens converter for top n

* Fix test

* Fix tests

* Fix comments

* Fix problem with timeseries

* Some fixes and refactoring

* Fix validation

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
@KOTungseth KOTungseth mentioned this pull request Oct 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flash1293 flash1293 flash1293 approved these changes

@ppisljar ppisljar ppisljar approved these changes

@stratoula stratoula Awaiting requested review from stratoula

@Kuznietsov Kuznietsov Awaiting requested review from Kuznietsov

Assignees

@VladLasitsa VladLasitsa

Labels

backport:skip This PR does not require backporting Feature:Lens Feature:TSVB TSVB (Time Series Visual Builder) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.5.0 WIP Work in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@VladLasitsa @elasticmachine @stratoula @flash1293 @kibana-ci @ppisljar @kibanamachine