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

Fix sorting of Monitoring nodes on CPU, Throttle, Heap columns #36125

Merged
merged 7 commits into from
May 23, 2019

Conversation

igoristic
Copy link
Contributor

@igoristic igoristic commented May 6, 2019

Summary

Resolves #36038

This feature: elastic/eui#929 is not really well documented, but it allows us to add sorting on values instead of object.

I checked all the tables in /monitoring/* and this was the only affected table

Checklist

@igoristic igoristic added bug Fixes for quality problems that affect the customer experience Team:Monitoring Stack Monitoring team v7.0.1 v7.1.0 labels May 6, 2019
@igoristic igoristic requested a review from chrisronline May 6, 2019 17:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@cjcenizal cjcenizal changed the title 36038 Fix sorting of Monitoring nodes on CPU, Throttle, Heap columns May 6, 2019
@cjcenizal
Copy link
Contributor

CC @chandlerprall @thompsongl Maybe there's a way to improve the EUI docs regarding this feature?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thompsongl
Copy link
Contributor

thompsongl commented May 6, 2019

Maybe there's a way to improve the EUI docs regarding this feature?

@cjcenizal I created elastic/eui#1919.

Feel free to add ideas or specific difficulties to that thread

@igoristic igoristic requested a review from a team as a code owner May 7, 2019 06:06
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded


const getSortHandler = (type) => (item) => _.get(item, `${type}.summary.lastVal`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why, but for this some reason, this isn't working for me.

const getSortHandler = (type) => (item) => _.get(item, [type, 'summary', 'lastVal']);

This does work for me and I don't know why. Can you confirm/deny?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump @igoristic. It would be great if we could get this resolved and then this fix merged. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: I didn't closely read the time stamp here and left the impression that this is urgent when it's not. Apologies if it came across like I was pushing for this to be done. It will be fine whenever you get to it. Thanks!

@ycombinator
Copy link
Contributor

ycombinator commented May 23, 2019

@igoristic I see you added the 7.1.0 label ~6 hours ago. 7.1.0 had already been released at that point: https://www.elastic.co/downloads/past-releases/kibana-7-1-0. So it could be misleading to think that this PR actually went out in 7.1.0.

I assume you plan to backport this fix to the 7.1 branch. At the moment that branch will be used for a 7.1.1 release but I see that label doesn't exist yet. You might want to chat with the Kibana team and create it or have them create it, then assign it here.

At any rate, the 7.1.0 label should definitely be removed, otherwise that can be misleading to users when they try to figure out which releases this fix went into.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work!

@igoristic igoristic merged commit 92ebb08 into elastic:master May 23, 2019
@igoristic igoristic deleted the 36038 branch May 23, 2019 17:29
igoristic added a commit to igoristic/kibana that referenced this pull request May 23, 2019
…ic#36125)

* Complete rebase

* Testing

* Retest

* Made the get method more compatible
@igoristic
Copy link
Contributor Author

igoristic commented May 23, 2019

Backport:
7.x: f23a1d7
6.8: 6ab2c0f

igoristic added a commit that referenced this pull request May 23, 2019
… (#37011)

* Complete rebase

* Testing

* Retest

* Made the get method more compatible
igoristic added a commit to igoristic/kibana that referenced this pull request Aug 20, 2019
…ic#36125)

* Complete rebase

* Testing

* Retest

* Made the get method more compatible
igoristic added a commit that referenced this pull request Aug 20, 2019
… (#43615)

* Complete rebase

* Testing

* Retest

* Made the get method more compatible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Team:Monitoring Stack Monitoring team v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitoring Nodes Sorting Broken on CPU, Throttle, Heap columns
7 participants