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

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

Merged
merged 14 commits into from
Aug 22, 2022

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Aug 5, 2022

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 Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens backport:skip This commit does not require backporting v8.5.0 WIP Work in progress labels Aug 5, 2022
@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
Contributor

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

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

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
Contributor

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
Contributor

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
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@stratoula, Could you please review again?

@stratoula
Copy link
Contributor

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
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor

flash1293 commented Aug 18, 2022

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

Copy link
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

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

@VladLasitsa
Copy link
Contributor Author

@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
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
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* 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 <[email protected]>
Co-authored-by: Joe Reuter <[email protected]>
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:TSVB TSVB (Time Series Visual Builder) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.5.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants