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.

Sign up for GitHub

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

Jump to bottom

[TSVB][Lens] Navigate to Lens with your current configuration #114794

Merged
merged 137 commits into from
Feb 14, 2022
Merged

[TSVB][Lens] Navigate to Lens with your current configuration #114794

merged 137 commits into from
Feb 14, 2022

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Oct 13, 2021 •
edited
Loading

Summary

Part of #115034
Closes #116424
This PR provides a new navigation item to TSVB that allows the users to navigate to Lens with the current configuration. The users can always return to TSVB without saving their Lens chart. If the user makes changes into the initial Lens chart, a modal should appear to notify the user about the changes.

image

This works for:

  • multiple layers
  • different layer types (bar, line, area)
  • custom palettes and colors
  • various aggregations:
    • Count
    • Average
    • Min
    • Max
    • Sum
    • Percentiles (creates different y axis per percentage)
    • Counter rate
    • Cumulative sum
    • Serial differences
    • Moving average
    • Overall max, min, sum, avg
    • Filter ratio
    • math
  • break down by terms aggregation and by filter(s)
  • different dataviews per layer
  • different timefield per layer
  • legend and grid options
  • time shift
  • custom interval
  • format per layer
  • custom labels
  • Axis extents

test

demo_lens2

PoC SOs for testing
PoC_SOs.zip

How the navigation works

  • Visualize editor -> TSVB --> Lens: The user can save the chart to library or embed it on a dashboard
  • Dashboard -> existing by ref TSVB --> Lens: The user can return to the dashboard and the TSVB panel is replaced by a Lens by value panel (TSVB chart still exists on the SOs)
  • Dashboard -> existing by value TSVB --> Lens: The user can return to the dashboard and the TSVB panel is replaced by a Lens by value panel
  • Dashboard -> new by value TSVB --> Lens: The user can return to the dashboard and a new Lens by value panel is added.

Cases that it doesn't work (nav item is disabled)

  • All non-timeseries charts
  • For all the aggregations that are not supported in Lens. This means that if I have two layers with one supported and one unsupported, the user can't navigate to Lens. In a next phase, we could possibly improve it by showing a modal to notify the users about the layers/configuration that won't be applied.

Flaky test runners

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/115 (50 times)
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/117 (50 times)

Notes

There might be cases that the 2 charts (Lens and TSVB) look a bit different. This is caused to various reasons. Some of them are:

  • Auto in TSVB and auto in Lens give a different interval
  • Missing values are depicted differently
  • Split by filters in Lens work with palette while in TSVB we assing a different color per series.

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula changed the title Convert to lens [TSVB][PoC] Navigate to Lens with your current configuration Oct 13, 2021
@elastic elastic deleted a comment from kibanamachine Oct 25, 2021
@elastic elastic deleted a comment from kibanamachine Oct 25, 2021
@flash1293 flash1293 mentioned this pull request Oct 27, 2021
Closed
@elastic elastic deleted a comment from kibanamachine Dec 6, 2021
@stratoula
Copy link
Contributor Author

stratoula commented Feb 11, 2022 •
edited
Loading

@MichaelMarcialis timerange never popups the appLeave modal. You can try it with a saved dashboard or a saved lens viz. (This is not introduced on this PR, this is a common behavior). This happens because the timerange is not part of the saved object. We don't save it because it is something dynamic.

@stratoula
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
lens 744 745 +1
visTypeTimeseries 291 298 +7
visualizations 198 207 +9
total +17

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
core 973 975 +2
lens 316 318 +2
navigation 31 32 +1
visualizations 296 320 +24
total +29

Async chunks

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

id before after diff
dashboard 293.1KB 293.2KB +22.0B
lens 1.0MB 1.1MB +10.4KB
visTypeTimeseries 451.3KB 451.4KB +18.0B
visualizations 151.4KB 157.2KB +5.8KB
total +16.3KB

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
lens 44 45 +1

Page load bundle

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

id before after diff
core 292.6KB 292.7KB +102.0B
lens 43.4KB 43.9KB +579.0B
navigation 8.7KB 8.9KB +281.0B
visTypeTimeseries 15.8KB 25.7KB +9.9KB
visualizations 42.9KB 43.4KB +562.0B
total +11.4KB
Unknown metric groups

API count

id before after diff
core 2366 2368 +2
lens 365 369 +4
navigation 31 32 +1
visualizations 316 341 +25
total +32

ESLint disabled line counts

id before after diff
visTypeTimeseries 17 18 +1

Total ESLint disabled count

id before after diff
visTypeTimeseries 20 21 +1

History

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

mbondyra
mbondyra approved these changes Feb 14, 2022
View reviewed changes
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.

Retested on Chrome, didn't notice anything new and all the raised problems are fixed 💯

flash1293
flash1293 reviewed Feb 14, 2022
View reviewed changes
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.

This looks really good! I tested around a bit and noticed a few small things we could support later on (but no big deal).

There's one thing that might make sense to address on this PR: For some reason it won't convert a math operation which uses the same variable twice:

Doesn't work
Screenshot 2022-02-14 at 10 26 42

Works:
Screenshot 2022-02-14 at 10 26 27

@stratoula
Copy link
Contributor Author

stratoula commented Feb 14, 2022 •
edited
Loading

Great catch @flash1293 ! Fixed 068b077

I saw your entries on our doc. I can create an issue to track them after this PR is merged and address them to a separate/followup PR.

ppisljar
ppisljar approved these changes Feb 14, 2022
View reviewed changes
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

@stratoula stratoula added the backport:skip This commit does not require backporting label Feb 14, 2022
flash1293
flash1293 approved these changes Feb 14, 2022
View reviewed changes
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 the only bug I found is fixed. Great work!

MichaelMarcialis
MichaelMarcialis approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for making that change, @stratoula! I've left you two small suggestions below, but otherwise this looks good to go once those are made. Approving now so I don't hold you up further.

src/plugins/navigation/public/top_nav_menu/top_nav_menu_item.tsx Outdated Show resolved Hide resolved
src/plugins/navigation/public/top_nav_menu/_index.scss Outdated Show resolved Hide resolved
@stratoula stratoula mentioned this pull request Feb 14, 2022
Closed
3 tasks
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 744 745 +1
visTypeTimeseries 291 298 +7
visualizations 198 207 +9
total +17

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
core 973 975 +2
lens 316 318 +2
navigation 31 32 +1
visualizations 296 320 +24
total +29

Async chunks

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

id before after diff
dashboard 293.1KB 293.2KB +22.0B
lens 1.0MB 1.1MB +10.4KB
visTypeTimeseries 451.3KB 451.4KB +18.0B
visualizations 151.4KB 157.2KB +5.8KB
total +16.3KB

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
lens 44 45 +1

Page load bundle

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

id before after diff
core 292.6KB 292.7KB +102.0B
lens 43.4KB 43.9KB +579.0B
navigation 8.7KB 9.3KB +644.0B
visTypeTimeseries 15.8KB 25.7KB +9.9KB
visualizations 42.9KB 43.4KB +562.0B
total +11.7KB
Unknown metric groups

API count

id before after diff
core 2366 2368 +2
lens 365 369 +4
navigation 31 32 +1
visualizations 316 341 +25
total +32

ESLint disabled line counts

id before after diff
visTypeTimeseries 17 18 +1

Total ESLint disabled count

id before after diff
visTypeTimeseries 20 21 +1

History

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

@stratoula stratoula merged commit d364f23 into elastic:main Feb 14, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 15, 2022
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into screenshotting/…
11c9878 
…pdf-generation-in-worker-thread

* 'main' of github.com:elastic/kibana: (40 commits)
  Service overview e2e test (elastic#125359)
  [Graph] Make graph edges easier to click (elastic#124053)
  [Reporting] Add additional PNG and PDF metrics  (elastic#125450)
  [APM] Lint rule for explicit return types (elastic#124771)
  [SecuritySolution] Enrich threshold data from correct fields (elastic#125376)
  [Uptime monitor management] Make index template installation retry (elastic#125537)
  [Console] Support suggesting index templates v2 (elastic#124655)
  [Logs UI] Support switching between log source modes (elastic#124929)
  SavedObjects management: change index patterns labels to data views (elastic#125356)
  [ci-stats] add Client class for accessing test group stats (elastic#125164)
  [ML] Use Discover locator in Data Visualizer instead of URL Generator (elastic#124283)
  [Fleet] Add retries when starting docker registry in integration tests (elastic#125530)
  Update osquery.asciidoc to address doc issue (elastic#125425)
  synthetics e2e - use custom location when defined (elastic#125560)
  [ci] Retry failed steps in on-merge pipeline (elastic#125550)
  [babel] Bump babel packages (elastic#125446)
  [DOCS] Updates Upgrade Assistant API status (elastic#125410)
  [DOCS] Minor tweaks to upgrade docs (elastic#125538)
  [Uptime] handle null duration on ping list (elastic#125438)
  [TSVB][Lens] Navigate to Lens with your current configuration (elastic#114794)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/export_types/common/pdf/pdfmaker.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf/lib/generate_pdf.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/generate_pdf.ts
@KOTungseth KOTungseth mentioned this pull request Mar 31, 2022
Merged
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
@drewdaemon drewdaemon mentioned this pull request Dec 7, 2022
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghudgins ghudgins ghudgins left review comments

@dej611 dej611 dej611 left review comments

@vadimkibana vadimkibana vadimkibana approved these changes

@ThomThomson ThomThomson ThomThomson approved these changes

@ppisljar ppisljar ppisljar approved these changes

@mbondyra mbondyra mbondyra approved these changes

@MichaelMarcialis MichaelMarcialis MichaelMarcialis approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@flash1293 flash1293 flash1293 approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment Feature:Lens Feature:TSVB TSVB (Time Series Visual Builder) release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Open in Lens for TSVB