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

show total in DataTable #4570

Merged
merged 5 commits into from
Jun 21, 2016
Merged

show total in DataTable #4570

merged 5 commits into from
Jun 21, 2016

Conversation

sonenko
Copy link
Contributor

@sonenko sonenko commented Aug 3, 2015

Any feedback encouraged :)

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@spalger
Copy link
Contributor

spalger commented Aug 3, 2015

My issue with this is that sum is only useful for counts, or maybe max metrics. If we could appropriately summarize metric values for different metric aggregations, and display those is a totals row I would be 👍

@@ -91,6 +92,12 @@ define(function (require) {
formattedColumn.class = 'visualize-table-right';
}

if (field.type === 'number') {
formattedColumn.total = table.rows.reduce(function (prev, curr, n, all) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use lodash's _.sum method here, maybe _.sum(table.rows, [i, 'value'])

@sonenko
Copy link
Contributor Author

sonenko commented Aug 3, 2015

@spalger Very good feedback 👍, thank you.
About sum do you mean one more request to server(or some tricky aggregation)? because in some cases we are not able calculate total from aggregated value?

@spalger
Copy link
Contributor

spalger commented Aug 3, 2015

I mean, for instance, if the column is averages of that row then the total value should probably be the average of the averages, not the sum of the averages.

@sonenko
Copy link
Contributor Author

sonenko commented Aug 3, 2015

hm. interesting idea. Will try to do it tomorrow. One more, do you think it will be handy if user will be able to choose column where to display total, instead of calculating total everywhere?
For me totals for some columns looks inappropriate, like total for index number.

@spalger
Copy link
Contributor

spalger commented Aug 5, 2015

@sonenko I think it makes sense to display the total for all of the metric aggregation columns. That way, if they want to choose a field to total up all they have to do is add a metric agg on that field :)

@sonenko
Copy link
Contributor Author

sonenko commented Aug 5, 2015

hm. ok. I will postpone this implementation till the weekend. Coz I started implementation for total for each column separately. Not need to rethink. So, your idea is to simply add one mode select component - Total function to Options tab?

@spalger
Copy link
Contributor

spalger commented Aug 5, 2015

Yeah, I think that would make it simple and cover 90% of the desired use cases. Do you agree?

@sonenko
Copy link
Contributor Author

sonenko commented Aug 5, 2015

I don't know:), I thought that Ideally, It would be more handy to configure this settings for each column. But from other perspective, it is just a little changes in code, and final user of kibana will not become confused because of many additional controls.

@sonenko
Copy link
Contributor Author

sonenko commented Aug 5, 2015

@spalger seems like I'm not able to use _.sum(Array, [key1, key2]);
Either I don't understand how _.sum works, or need to improve _.sum :)

@sonenko
Copy link
Contributor Author

sonenko commented Aug 7, 2015

could anybody please review that?

@rashidkpc
Copy link
Contributor

There's more to this than just sum or avg, there's also min, max and probably a whole lot more. I'm not sure how to come at this in a generic way

@sonenko
Copy link
Contributor Author

sonenko commented Aug 20, 2015

I will try to add few more aggregations on the next week.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@spalger
Copy link
Contributor

spalger commented Apr 5, 2016

jenkins, test it

@panda01
Copy link
Contributor

panda01 commented Jun 10, 2016

jenkins, test it.

@panda01 panda01 added the review label Jun 10, 2016
@epixa epixa removed the P3 label Jun 13, 2016
@epixa
Copy link
Contributor

epixa commented Jun 13, 2016

This needs to be merged with or rebased on the latest master.

@panda01 panda01 self-assigned this Jun 13, 2016
@sonenko
Copy link
Contributor Author

sonenko commented Jun 13, 2016

jenkins, test it.

@epixa
Copy link
Contributor

epixa commented Jun 14, 2016

jenkins, test it

<select ng-disabled="!vis.params.showTotal"
class="form-control"
ng-model="vis.params.totalFunc"
ng-options="x for x in ['sum', 'avg', 'min', 'max', 'count']">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a variable on the scope please?

@sonenko
Copy link
Contributor Author

sonenko commented Jun 14, 2016

@panda01 thank you for fast reaction

@panda01
Copy link
Contributor

panda01 commented Jun 14, 2016

For the record;
table-totals

@panda01
Copy link
Contributor

panda01 commented Jun 14, 2016

This looks good to me. Although in all honesty I'm not sure if that's all the functions we want, and if they're calculated as we would like them to. Passing on to @spalger, or @rashidkpc for some feedback in regards to these issues.

@panda01
Copy link
Contributor

panda01 commented Jun 14, 2016

Jenkins, test it.

@sonenko
Copy link
Contributor Author

sonenko commented Jun 14, 2016

Thanks @panda01

@sonenko
Copy link
Contributor Author

sonenko commented Jun 17, 2016

Hi @spalger, @rashidkpc, could you look at that please?

@panda01
Copy link
Contributor

panda01 commented Jun 21, 2016

Actually, I changed my mind. I will take responsibility for merging it, if there are any problems i will handle them accordingly. Thanks for all of your work @sonenko!

@panda01 panda01 merged commit c6f5963 into elastic:master Jun 21, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
show total in DataTable

Former-commit-id: c6f5963
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants