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.

Sign up for GitHub

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

Jump to bottom

Add support for adding aliases to index and shard stats #563

Merged
merged 4 commits into from
May 19, 2022
Merged

Add support for adding aliases to index and shard stats #563

merged 4 commits into from
May 19, 2022

Conversation

bobo333
Copy link
Contributor

@bobo333 bobo333 commented May 6, 2022

Optionally will include aliases in a label to index and shard stats. Addresses #412

This adds a cli option es.aliases which when enabled, will add all aliases pointing to a given index, comma-separated, into a label called aliases for index and shard stats.

ex:
3 indices are created, foo_1, foo_2, and foo_3. foo_2 has one alias pointing to it called foo_alias_2_1. foo_3 has 2 aliases pointing to it called foo_alias_3_1 and foo_alias_3_2.

without enabling es.aliases flag:

...
elasticsearch_indices_store_size_bytes_total{cluster="docker-cluster",index="foo_1"} 4518
elasticsearch_indices_store_size_bytes_total{cluster="docker-cluster",index="foo_2"} 4564
elasticsearch_indices_store_size_bytes_total{cluster="docker-cluster",index="foo_3"} 4564
...

with es.aliases flag:

...
elasticsearch_indices_store_size_bytes_total{aliases="",cluster="docker-cluster",index="foo_1"} 4518
elasticsearch_indices_store_size_bytes_total{aliases="foo_alias_2_1",cluster="docker-cluster",index="foo_2"} 4564
elasticsearch_indices_store_size_bytes_total{aliases="foo_alias_3_1,foo_alias_3_2",cluster="docker-cluster",index="foo_3"} 4564
...

@bobo333
Copy link
Contributor Author

bobo333 commented May 12, 2022

@sysadmind @SuperQ wondering if you had any thoughts or feedback on this

sysadmind
sysadmind requested changes May 13, 2022
View reviewed changes
Copy link
Contributor

@sysadmind sysadmind left a comment

Choose a reason for hiding this comment

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

Having variable labels is not a good idea. I understand what the intent is, and I think that having aliases exposed is a good idea. I don't think that they belong in a label of the existing metrics however.

I think it would be better to have a new metric elasticsearch_indices_alias with two labels index and alias. The scarping of the aliases would stay the same, but the reporting of the metric would be a loop over the indices and aliases and to expose the above gauge for each index-alias combination with a value of 1. This is a common pattern in prometheus for informational metrics.

How would this help your use case? The alias information would come at query time. You can use your normal query but add group_left(index) to get the alias label into your existing queries.

@bobo333
Copy link
Contributor Author

bobo333 commented May 15, 2022

@sysadmind thank you very much for the review! I will get started on those changes. Out of curiosity - what is the reason for not wanting aliases in labels of existing metrics? From the end user perspective it seems it would be easier to filter on a label rather than doing group_left, however I imagine you have a good reason for suggesting that route. Thanks again!

@SuperQ
Copy link
Contributor

SuperQ commented May 15, 2022

The main issue is that label values in Prometheus are opaque strings. So you end up with problems trying to do one-to-many relationships. Mapping a list of aliases to a coma separate list is not as useful as mapping to an information metric.

Take your example, it's now much easier to match the literal single alias depend on which one you're looking for:

elasticsearch_indices_store_size_bytes_total{cluster="docker-cluster",index="foo_3"}
...
elasticsearch_indices_aliases{alias="foo_alias_3_1",cluster="docker-cluster",index="foo_3"} 1
elasticsearch_indices_aliases{alias="foo_alias_3_2",cluster="docker-cluster",index="foo_3"} 1

@bobo333
Copy link
Contributor Author

bobo333 commented May 16, 2022

@SuperQ that makes sense, thank you for explaining!

one last question - would you like the addition of the alias information metrics to be optionally enabled by a cli option, or always enabled? thanks again!

@SuperQ
Copy link
Contributor

SuperQ commented May 16, 2022

I think we could enable it by default, with a flag to disable it if it generates too many metrics for some users.

bobo333 added 2 commits May 17, 2022 17:07
@bobo333
Add support for adding aliases to index and shard stats
220f7a2 
Optionally will include aliases in a label to index and shard stats

Signed-off-by: Steven Cipriano <cipriano@squareup.com>
@bobo333
move aliases to separate informational metrics
4232fd7 
Signed-off-by: Steven Cipriano <cipriano@squareup.com>
@bobo333
Merge branch 'master' into add-aliases
9ec16c7 
* master:
  Refactor cluster info collector (prometheus-community#536)
  Fix linting that was missed in CI run (prometheus-community#568)
  Grafana dashboard: use new node exporter metric names (prometheus-community#501)
  publish total shards on a node (prometheus-community#535)
  Add additional collector for SLM stats (prometheus-community#558)
  Update common Prometheus files (prometheus-community#565)
  Update build (prometheus-community#562)

Signed-off-by: Steven Cipriano <cipriano@squareup.com>
@bobo333
Copy link
Contributor Author

bobo333 commented May 18, 2022

@SuperQ @sysadmind this is ready for another look, thanks!

example stats:

...
# HELP elasticsearch_indices_aliases Record aliases associated with an index
# TYPE elasticsearch_indices_aliases gauge
elasticsearch_indices_aliases{alias="foo_alias_2_1",cluster="docker-cluster",index="foo_2"} 1
elasticsearch_indices_aliases{alias="foo_alias_3_1",cluster="docker-cluster",index="foo_3"} 1
elasticsearch_indices_aliases{alias="foo_alias_3_2",cluster="docker-cluster",index="foo_3"} 1
...

SuperQ
SuperQ reviewed May 18, 2022
View reviewed changes
main.go Outdated Show resolved Hide resolved
@bobo333
address review
d2c4293 
- switch boolean flag
- fix linting errors

Signed-off-by: Steven Cipriano <cipriano@squareup.com>
sysadmind
sysadmind approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@sysadmind sysadmind left a comment

Choose a reason for hiding this comment

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

LGTM I think this is ok now

@sysadmind sysadmind merged commit b536294 into prometheus-community:master May 19, 2022
@SuperQ
Copy link
Contributor

SuperQ commented May 19, 2022

Nice

iishabakaev pushed a commit to iishabakaev/elasticsearch_exporter that referenced this pull request Jun 8, 2022
@bobo333 @iishabakaev
Add support for adding aliases to index and shard stats (prometheus-c…
8f95b31 
…ommunity#563)

* Add support for adding aliases to index and shard stats

Optionally will include aliases in a label to index and shard stats

Signed-off-by: Steven Cipriano <cipriano@squareup.com>

* move aliases to separate informational metrics

Signed-off-by: Steven Cipriano <cipriano@squareup.com>

* address review

- switch boolean flag
- fix linting errors

Signed-off-by: Steven Cipriano <cipriano@squareup.com>
Signed-off-by: iishabakaev <iishabakaev@gmail.com>
jnadler pushed a commit to jnadler/elasticsearch_exporter that referenced this pull request Oct 27, 2022
@bobo333
Add support for adding aliases to index and shard stats (prometheus-c…
055f2c4 
…ommunity#563)

* Add support for adding aliases to index and shard stats

Optionally will include aliases in a label to index and shard stats

Signed-off-by: Steven Cipriano <cipriano@squareup.com>

* move aliases to separate informational metrics

Signed-off-by: Steven Cipriano <cipriano@squareup.com>

* address review

- switch boolean flag
- fix linting errors

Signed-off-by: Steven Cipriano <cipriano@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ left review comments

@sysadmind sysadmind sysadmind approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bobo333 @SuperQ @sysadmind