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 cluster label to shard metric #639

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Add cluster label to shard metric #639

merged 1 commit into from
Dec 12, 2023

Conversation

arpitjindal97
Copy link
Contributor

Almost all the other metrics have this label, so it becomes a norm to have it on this metric as well

@arpitjindal97
Copy link
Contributor Author

@sysadmind Can you please have a look at this?

sysadmind
sysadmind reviewed Nov 1, 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.

There are a lot of metrics that don't include the cluster name. I'm not sure that it makes sense to create a Nodes{} object and pull that data again just to add a label. I think to accept this we would need a better way to share that data.

collector/shards.go Show resolved Hide resolved
@arpitjindal97
Copy link
Contributor Author

So, we have like 100 of ElasticSearch clusters and we are running this exporter in each node and collecting metrics in a single prometheus storage.

We want cluster label so that we can distinguish between multiple clusters. We have one grafana dashboard with cluster as a variable which we are passing in queries. This specific metric doesn't have it, so I raised the Pull Request.

@sysadmind
Copy link
Contributor

I'm on board with this change, but I would like to remove the dependency on Nodes to get the cluster name. There is some existing work to have collectors receive updates to the cluster info: https://github.com/prometheus-community/elasticsearch_exporter/blob/master/main.go#L208. It's not the prettiest functionality, but it will cut down on API calls for those user that care about that type of thing. It would also mean if we refactor in the future, we have a common set of functionality to consider.

@arpitjindal97
Copy link
Contributor Author

@sysadmind I have updated the code. Please review

@arpitjindal97
Copy link
Contributor Author

@SuperQ Can you please review it?

@arpitjindal97 arpitjindal97 mentioned this pull request Oct 15, 2023
Merged
SuperQ
SuperQ requested changes Oct 15, 2023
View reviewed changes
collector/shards.go Show resolved Hide resolved
sysadmind
sysadmind requested changes Oct 15, 2023
View reviewed changes
collector/shards.go Show resolved Hide resolved
collector/shards.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@sysadmind sysadmind mentioned this pull request Oct 15, 2023
Open
@arpitjindal97
Copy link
Contributor Author

@sysadmind

The code actually uses a JSON representation, so those arrows would not be relevant.

I didn't understand this, can you explain a bit more

@sysadmind
Copy link
Contributor

Your comment/screenshot is for _cat/shards, but the exporter doesn't use this URL in that way. Your screenshot shows data in a formay that the exporter doesn't see. The URL that the exporter uses is _cat/shards?format=json

@arpitjindal97
Copy link
Contributor Author

@sysadmind I have added the screenshots from correct API.

@arpitjindal97
Copy link
Contributor Author

@sysadmind @SuperQ Please review

SuperQ
SuperQ previously requested changes Oct 21, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@arpitjindal97
Add cluster label to shard metric
32da1e5 
 - Almost all the other metrics have this label, so it becomes a norm to
   have it on this metric as well
 - Only capture shards with state "STARTED"

Signed-off-by: Arpit Agarwal <[email protected]>
@arpitjindal97
Copy link
Contributor Author

@sysadmind @SuperQ Gentle reminder

sysadmind
sysadmind approved these changes Dec 12, 2023
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. The remaining feedback is all around the refactoring of collectors that we are working on, so that can be addressed as part of that effort.

@sysadmind sysadmind dismissed SuperQ’s stale review December 12, 2023 18:05

Stale / addressed

@sysadmind sysadmind merged commit 9fd3634 into prometheus-community:master Dec 12, 2023
1 check passed
sysadmind added a commit to sysadmind/elasticsearch_exporter that referenced this pull request Dec 12, 2023
@sysadmind
Add prometheus-community#639
5ba2346 
Signed-off-by: Joe Adams <[email protected]>
sysadmind added a commit that referenced this pull request Dec 21, 2023
@sysadmind
Add changelog for v1.7.0 (#827)
d24c834 
* Add changelog for v1.7.0

This includes requested security upgrades with dependencies.

Signed-off-by: Joe Adams <[email protected]>

* Add 816

Signed-off-by: Joe Adams <[email protected]>

* Add #639

Signed-off-by: Joe Adams <[email protected]>

---------

Signed-off-by: Joe Adams <[email protected]>
jaimeyh pushed a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@arpitjindal97 @jaimeyh
Add cluster label to shard metric (prometheus-community#639)
b1136b2 
- Almost all the other metrics have this label, so it becomes a norm to
   have it on this metric as well
 - Only capture shards with state "STARTED"

Signed-off-by: Arpit Agarwal <[email protected]>
jaimeyh pushed a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@sysadmind @jaimeyh
Add changelog for v1.7.0 (prometheus-community#827)
efe88f2 
* Add changelog for v1.7.0

This includes requested security upgrades with dependencies.

Signed-off-by: Joe Adams <[email protected]>

* Add 816

Signed-off-by: Joe Adams <[email protected]>

* Add prometheus-community#639

Signed-off-by: Joe Adams <[email protected]>

---------

Signed-off-by: Joe Adams <[email protected]>
jaimeyh added a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@jaimeyh
Revert "Add cluster label to shard metric (prometheus-community#639)"
6c24229 
This reverts commit b1136b2.
jaimeyh added a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@jaimeyh
Revert "Add cluster label to shard metric (prometheus-community#639)"
e8240bc 
This reverts commit b1136b2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sysadmind sysadmind sysadmind approved these changes

@SuperQ SuperQ Awaiting requested review from SuperQ

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
@arpitjindal97 @sysadmind @SuperQ