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

Simplify nodes stats API #4347

Merged

Conversation

spinscale
Copy link
Contributor

First, this breaks backwards compatibility!

  • Removed /_cluster/nodes/stats endpoint
  • Excpect the stats types not as parameters, but as part of the URL
  • Returning all indices stats by default, returning all nodes stats by default
  • Supporting groups & types in nodes stats now as well
  • Updated documentation & tests accordingly

Closes #4057

@kimchy
Copy link
Member

kimchy commented Dec 8, 2013

LGTM, I would love @clintongormley to try it out as well...

@clintongormley
Copy link

Heya

This doesn't seem to support the indices-metrics, eg:

/_nodes/stats/indices/indexing

Also doesn't support the level parameter

@spinscale
Copy link
Contributor Author

Fixed the indices metrics.

Regarding supporting the level parameter: The aggregation of information happens to early right now, preventing the stats from being returned per shard at all (for the curious: InternalIndicesService.stats() loops through the index shards and aggregates there already.

Requires some refactoring around the NodeIndicesStats class to support shard level stats and aggregate them later.

@spinscale
Copy link
Contributor Author

@clintongormley added the level parameter for "indices" and "shards", however "cluster" does not make a lot of sense in a response, which is per node...

will squash it into one commit after review

@clintongormley
Copy link

True... perhaps should be node, indices, shards ?

@spinscale
Copy link
Contributor Author

I like it, wait with reviewing, will put that in first

@bleskes
Copy link
Contributor

bleskes commented Dec 12, 2013

Hi,

I've had a quick look to see how things will impact the cluster stats end point I work on now. Things look good from that perspective (it will even allow for removing some code in the cluster stats end point).

Two comments:

  • the indices stats API return the index level stats both if level=indices (default) & level=shards , which is different than here.
  • Wording wise - level=indices implies to me a per-index stats (which we don't expose now at all). I like Clint's suggestion of having node (what we do now in 0.90), indices (per index) and shards (per shard).

Perhaps we want to make level multi-valued. If we do so, we allow people to get both aggregates and detailed level if need be.

@spinscale
Copy link
Contributor Author

@bleskes I will also create the additive behaviour here when the level parameter is specified. Wording wise I will go with node (default as it is right now), indices (per index) and shards

thx for the input

@spinscale
Copy link
Contributor Author

Pushed to support node, indices and shards as level parameter (documentation still pending) - would be happy about another enduser test

First, this breaks backwards compatibility!

* Removed /_cluster/nodes/stats endpoint
* Excpect the stats types not as parameters, but as part of the URL
* Returning all indices stats by default, returning all nodes stats by default
* Supporting groups & types in nodes stats now as well
* Updated documentation & tests accordingly
* Allow level parameter for "shards" and "indices" (cluster does not make sense here)

Closes elastic#4057
@spinscale spinscale merged commit bb27516 into elastic:master Jan 6, 2014
karmi added a commit to elastic/elasticsearch-ruby that referenced this pull request Jan 11, 2014
…` to `/_nodes`

The "_cluster" prefix has been removed in Elasticsearch 1.0.

The "/_nodes" endpoint is supported since ~ version 0.19,
so this doesn't break backwards compatibility.

Related:

* elastic/elasticsearch#4055
* elastic/elasticsearch#4065
* elastic/elasticsearch#4347
* elastic/elasticsearch#4608
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.

Changes to the nodes stats API
4 participants