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] Refactor Table Visualization #15747

Closed
wants to merge 2 commits into from
Closed

[TSVB] Refactor Table Visualization #15747

wants to merge 2 commits into from

Conversation

simianhacker
Copy link
Member

This PR refactors the table visualization in TSVB with the following changes:

  • Use partitioning to get all rows
  • Add sorting to all columns
  • Add pagination controls
  • Convert table to use EUI
  • Add Kibana config to control the total limit of rows
  • Add Kibana config to control the partition size

image

Merry Xmas @alexfrancoeur !

@simianhacker simianhacker added Feature:TSVB TSVB (Time Series Visual Builder) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.2.0 v7.0.0 labels Dec 21, 2017
@simianhacker simianhacker force-pushed the tsvb-table-part-2 branch 2 times, most recently from b517f95 to 9f41f7e Compare December 22, 2017 00:05
shaharmor
shaharmor reviewed Dec 22, 2017
View reviewed changes
src/core_plugins/metrics/index.js
@@ -15,7 +15,9 @@ export default function (kibana) {
return Joi.object({
enabled: Joi.boolean().default(true),
chartResolution: Joi.number().default(150),
minimumBucketSize: Joi.number().default(10)
minimumBucketSize: Joi.number().default(10),
tablePartitionSize: Joi.number().default(50),
Copy link
Contributor

@shaharmor shaharmor Dec 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why make these configuration options global? I think it would be much more flexible if it were on a per table basis instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let’s do both... I think admins should have a way to put safe guards on their systems.

@simianhacker simianhacker mentioned this pull request Dec 24, 2017
Closed
@alexfrancoeur
Copy link

woah! what a gift @simianhacker!! Just checked out the PR, couple of comments below. Hope this helps, feel free to ping me with any questions.

screen shot 2017-12-27 at 10 27 35 am

  • If there are not enough rows on a table to fill the panel, the controls and page number seem to be directly under the table rather than at the bottom of the dashboard panel. Is there a way to make sure the controls are the bottom of the visualization / dashboard panel rather than directly under the table?

  • Do we need to show a page number if there is only one page?

  • Also, ideally the number of rows to show on a page would be an edit function for a dashboard panel. Though we'd need to some input from the dashboard team here. The only reason I bring is up is because I know eventually some users are going to say that this takes up too much space. But honestly, if we're showing page numbers they end up being in line with one another anyway

screen shot 2017-12-27 at 10 34 58 am

screen shot 2017-12-27 at 10 35 07 am

  • Text in input boxes and dropdowns seem to be different sizes and styles. Is this a side effect of using EUI?

  • I keep getting a fatal error when switching between any other TSVB viz type and back to the table. The result is an automatic refresh so I can't capture the error.

dec-27-2017 10-47-59

  • Scrollbar isn't shown by default so I can't determine if there are extra rows / pages or not. We could pull these controls out of the data table so that they are always visible and not just directly below the data table.

screen shot 2017-12-27 at 10 31 54 am

  • An info button or something describing exactly how we're determining a "trend". I've made incorrect assumptions here myself, I imagine our users will as well (and eventually will probably want to customize). IE, trend up if it's greater than the last bucket. Trend up if it's greater than this value from 24 hours ago, etc.

screen shot 2017-12-27 at 10 33 51 am

  • I'm still not sure what field and aggregate function do here. What are they used for?

screen shot 2017-12-27 at 10 43 33 am

  • Using a weeks worth of IP's and a derivative calculation with daily intervals, I'm seeing a 1.2MB and 4.47s response time. Note too bad, but noticeable. How does this perform at scale? To be safe, maybe we still offer a top X option here. Thoughts?

@uboness
Copy link

uboness commented Dec 27, 2017

Re the table... We should have another table pagination component in eui to use for this purpose. One that:

  • doesn't show page numbers and instead only shows next & prev buttons
  • also it should be possible to opt-out of the page size selector and instead try to compute the page size dynamically based on the available widget area (this might need to be done in a dedicated table component that we'll build specifically for embedding as a widget in a dashboard)

For now though... With the current (and only) eui table pagination component... A single page is still showing as a button.. To change this behavior we'll need to change it in eui, not here.... Though I wouldn't change it... Instead I would just ask to build another pagination component that is better suited for this scenario (as I mentioned above)

@alexfrancoeur
Copy link

@uboness thanks for the additional info, I'll sync with @snide and team on the table component.

@simianhacker let me know if you have any questions around my other feedback

@jimgoodwin jimgoodwin added v6.2.3 and removed v6.2.2 labels Feb 20, 2018
@jimgoodwin jimgoodwin added v6.2.4 and removed v6.2.3 labels Mar 20, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker simianhacker mentioned this pull request Mar 29, 2018
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker simianhacker force-pushed the tsvb-table-part-2 branch 3 times, most recently from 0ffc28a to acce848 Compare April 10, 2018 23:59
@simianhacker
Copy link
Member Author

@timroes I can re-produce this behavior when I increase the time frame to the point that the auto bucketing produces a bucket for the last value which contains all of the data (which get's dropped and displays zero). If you go under the panel options and turn off "drop last bucket" the values should display properly. PR #15760 introduces a new "drop partial buckets" behavior that only drops the last bucket if it's a partial bucket.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Apr 11, 2018

@simianhacker I think I am getting a different failure. a) I have data for at least 30 days in there and getting that when selecting the last 7 days, so there should still be data outside of the last bucket. b) With just one Count column it draws perfectly fine for that time range, and it also shows, that there are several documents found:

screenshot-20180411-110934

But as soon as I add another column if switches to that error, unless I switch the timerange to something small (without data?): (Sorry for the flickering in the GIF, but Linux and GIF recording....)

tsvb-table

So I think there should still be documents available to draw that.

@alexfrancoeur
Copy link

@timroes @simianhacker I am also able to reproduce

apr-11-2018 08-15-10

@simianhacker
[TSVB] Refactor Table Visualization
c995dc4 
- Use partitioning to get all rows
- Add sorting to all columns
- Add pagination controls
- Convert table to use EUI
- Add Kibana config to control the total limit of rows
- Add Kibana config to control the partition size
- Adding check for data
- Converting es5 functions to es6 because prettier disagrees with our ES lint, why are we still using eslint with prettier?
- Only show the pagination if there are enough rows to justify it.
- Adding check for compatible data type
@simianhacker
Copy link
Member Author

@timroes @alexfrancoeur This has to do with how the drop last bucket feature works and how sparse the data is. PR #15760 replaces the drop last bucket feature with a drop partial bucket feature, which I think will fix some of the data issues you're seeing.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@LeeDr LeeDr added v6.5.0 and removed v6.4.0 labels Jul 25, 2018
@timroes timroes removed their request for review August 30, 2018 09:15
@simianhacker simianhacker mentioned this pull request Oct 18, 2018
Closed
@spalger spalger added v6.6.0 and removed v6.5.0 labels Oct 25, 2018
@snide snide added v7.0.1 and removed v7.0.0 labels Apr 10, 2019
@simianhacker
Copy link
Member Author

I'm closing this PR because it's been open for too long and would need a non-trivial amount of work to fix.

@simianhacker simianhacker deleted the tsvb-table-part-2 branch April 17, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaharmor shaharmor shaharmor left review comments

@ppisljar ppisljar Awaiting requested review from ppisljar

Assignees
No one assigned
Labels
Feature:TSVB TSVB (Time Series Visual Builder) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.7.0 v7.0.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.