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

Ensure extra columns are not shown in table vis when showPartialRows:true #25690

Merged
merged 8 commits into from
Dec 18, 2018

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Nov 14, 2018

Closes #22168

Summary

Resolves a regression where clicking show partial rows on the data table vis would automatically render columns with metrics at all levels -- even if you didn't check metrics at all levels.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

* @param {boolean} respOpts.metricsAtAllLevels - reflects the value of vis.params.showMetricsAtAllLevels
* @param {boolean} respOpts.partialRows - reflects the value of vis.params.showPartialRows
* @param {Object} respOpts.timeRange - time range object, if provided
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

Since params are being thrown around willy-nilly and hard to track, I added this to reduce confusion for people working on Tabify in the future. (A TypeScript rewrite would be very helpful here).

@@ -133,7 +134,8 @@ const CourierRequestHandlerProvider = function () {

const parsedTimeRange = timeRange ? getTime(aggs.indexPattern, timeRange) : null;
const tabifyParams = {
metricsAtAllLevels: isHierarchical,
isHierarchical,
metricsAtAllLevels,
Copy link
Member Author

Choose a reason for hiding this comment

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

These are split into separate params, because within table vis there is a distinction between the two. For example, the following is possible:

vis.params.showMetricsAtAllLevels === false;
vis.params.showPartialRows === true;
vis.isHierarchical(); // still returns true

This is due to how we configure the table vis:
https://github.com/elastic/kibana/blob/9b08fc719a9fb4c0c1dce55be73d395e7cd6b54a/src/core_plugins/table_vis/public/table_vis.js#L114-L116

Copy link
Member

Choose a reason for hiding this comment

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

this is actually wrong i think ....
hierarchical === metricsAtAllLevels (unless someone knows of any reasons it should be ? )

in courier request handler vis.isHierarchical() is used to figure out if we need to build a request with metrics on every level. (or it was, and its now refactored to look at metricsAtAllLevels parameter). we should use this same parameter in tabify (to figure out the same thing) ...
and partialRows should be completely independent ... as its just removing rows where we don't have value for every column.

@lukeelmers lukeelmers added v7.0.0 v6.4.4 v6.5.1 Feature:Data Table Data table visualization feature labels Nov 14, 2018
@lukeelmers lukeelmers added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Nov 14, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukeelmers
Copy link
Member Author

^^ seems to be a legit failure, looking into it

Uncaught TypeError: Cannot read property 'value' of undefined

in build_hierarchical_data.js

@lukeelmers
Copy link
Member Author

Next build should be green 🤞

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@trevan
Copy link
Contributor

trevan commented Nov 15, 2018

I tested this and it looks good.

@lukeelmers
Copy link
Member Author

Thanks for reviewing @trevan! Much appreciated

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.

i think this introduces some more confusion with metricsAtAllLevels and isHierarchical ...

hierarchical === metricsAtAllLevels (unless someone knows of any reasons it should be ? )

in courier request handler vis.isHierarchical() is used to figure out if we need to build a request with metrics on every level. (or it was, and its now refactored to look at metricsAtAllLevels parameter). we should use this same parameter in tabify (to figure out the same thing) ...
and partialRows should be completely independent ... as its just removing rows where we don't have value for every column.

src/ui/public/agg_response/tabify/_response_writer.js Outdated Show resolved Hide resolved
src/ui/public/agg_response/tabify/_response_writer.js Outdated Show resolved Hide resolved
@LeeDr LeeDr added v6.5.3 and removed v6.5.2 labels Nov 29, 2018
@lukeelmers lukeelmers force-pushed the fix/table-partial-rows branch from d032949 to ab544ed Compare December 4, 2018 16:25
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers
Copy link
Member Author

lukeelmers commented Dec 4, 2018

@ppisljar I renamed some of the params that are being passed around to reduce confusion.

Rather than using the showMetricsAtAllLevels vis param, which pretty much only exists for data table, I created a columnsForAllBuckets arg for tabify that specifically exists to indicate whether we need to have tabify return a column for each bucket/level.

I moved the logic for checking the existence of params.showMetricsAtAllLevels to the visualize data loader, which feels more appropriate than having tabify concern itself with the details of the vis.

The overall effect is the same, but hopefully this makes its purpose clearer so it doesn't get mixed up with isHierarchical.

EDIT: We also used to have a minimalColumns setting -- this is essentially what I was trying to do with columnsForAllBuckets

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukeelmers
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukeelmers lukeelmers force-pushed the fix/table-partial-rows branch from 440d4f2 to b999e66 Compare December 13, 2018 19:25
@lukeelmers lukeelmers requested a review from a team as a code owner December 13, 2018 19:25
@lukeelmers lukeelmers changed the base branch from master to 6.x December 13, 2018 19:25
@lukeelmers lukeelmers added v6.5.4 and removed v7.0.0 labels Dec 13, 2018
@elastic elastic deleted a comment from elasticmachine Dec 13, 2018
@elastic elastic deleted a comment from elasticmachine Dec 13, 2018
@lukeelmers
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers added v6.5.5 and removed v6.5.4 labels Dec 17, 2018
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Don't know why we got pinged on this one. Normally it's if the Pr has Sass files. In any case, nothing looks off by me.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

LGTM, left some minor suggestions. @trevan if you want to give this a final review if it satisfies your needs too, please feel free

@trevan
Copy link
Contributor

trevan commented Dec 17, 2018

@timroes, it looks good

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Table Data table visualization feature Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.5.5 v6.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants