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

Modify load average format #15932

Merged
merged 1 commit into from Jan 18, 2016

Conversation

Projects
None yet
3 participants
@jasontedor
Copy link
Member

commented Jan 12, 2016

This commit modifies the load_average in the node stats API response
to be an object containing the one-minute, five-minute and
fifteen-minute load averages as fields (if those values are
available). Additionally, this commit modifies the cat nodes API
response to format the one-minute, five-minute and fifteen-minute load
averages as null if any of the respective values are not available.

Relates #15907

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

This solution seems very specific. @tlrx was working on a more generic solution here #13708

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2016

@clintongormley Recapping a few conversations we've had through other channels.

This solution seems very specific.

It is, but it's restoring a minor feature that was in the 1.x line of Elasticsearch when Elasticsearch made available the one-minute, five-minute, and fifteen-minute load averages. In the 2.x line of Elasticsearch, only the one-minute load average is available and is presented as a number. In #15907, the five-minute and fifteen-minute load averages were brought back, but presenting the values as an array without the ability to change the format. This was guided initially by the default from the 1.x line, but I'm now thinking the object format is more useful than the array format.

@tlrx was working on a more generic solution here #13708

For now, this is stalled by other priorities and unlikely to make the next major release of Elasticsearch.

Given this, I think that either of the following outcomes is reasonable.

  1. take the pull request as is, but add a TODO to revisit pending #13708
  2. modify the load_average field to present it as
   load_average: {
     "1m": 6.21,
     "5m": 7.01,
     "15m": 6.33
   }

by default and not have the option to present them as an array

@clintongormley What do you think?

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

I like the object notation - makes more sense to me

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2016

I like the object notation - makes more sense to me

@clintongormley Agree, so I think it should be the default, but where does that leave us on the ability to format as an array given that it was possible in 1.x but lost in 2.x and has caused some frustration?

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

@jasontedor my personal take would be to support the object format only. The array format isn't useful for anybody. @NickCraver would you agree?

@NickCraver

This comment has been minimized.

Copy link

commented Jan 18, 2016

The array is what we'd expect (and always how you see it in a system) - but if it varies across platforms then an object makes more sense. However, the more important thing is is stability in these monitoring APIs since they must be used across minor and major versions in all the places they are used. To support one value being formatted as an array, then an integer, then an object is nuts for static deserialization.

In porting our monitoring to support 2.0, this was 1 of 2 breaking changes in that regard. I don't think the stability is given as much attention as it deserves on these issues.

@jasontedor jasontedor changed the title Add query string field to nodes stats API to format load_average Modify load average format Jan 18, 2016

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

@clintongormley I've updated this pull request to remove the array format and only format the load average as an object (values that are not available will not appear in the response).

On OS X:

$ curl -sS -XGET localhost:9200/_nodes/stats/os \
> | jq '.nodes[].os.cpu.load_average'

{
  "1m": 0.55712890625
}

On Linux:

$ curl -sS -XGET localhost:9200/_nodes/stats/os \
> | jq '.nodes[].os.cpu.load_average'

{
  "1m": 2.81,
  "5m": 7.12,
  "15m": 7.2
}
@clintongormley

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

In porting our monitoring to support 2.0, this was 1 of 2 breaking changes in that regard. I don't think the stability is given as much attention as it deserves on these issues.

Yes I get that - we didn't think it through clearly enough, but we can't change history so we're looking for the best path forward.

I've updated this pull request to remove the array format and only format the load average as an object

@jasontedor I think this is good - it works across any system, including indexing back into Elasticsearch itself. thanks

Modify load average formats
This commit modifies the load_average in the node stats API response
to be an object containing the one-minute, five-minute and
fifteen-minute load averages as fields (if those values are
available). Additionally, this commit modifies the cat nodes API
response to format the one-minute, five-minute and fifteen-minute load
averages as null if any of the respective values are not available.

@jasontedor jasontedor removed the review label Jan 18, 2016

jasontedor added a commit that referenced this pull request Jan 18, 2016

@jasontedor jasontedor merged commit 3be12ed into elastic:master Jan 18, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jasontedor jasontedor deleted the jasontedor:load-average-format branch Jan 18, 2016

jasontedor added a commit that referenced this pull request Jan 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.