Skip to content

rework prometheus#15

Open
tylergmuir wants to merge 1 commit intotenortim:mainfrom
tylergmuir:cleanup-prometheus
Open

rework prometheus#15
tylergmuir wants to merge 1 commit intotenortim:mainfrom
tylergmuir:cleanup-prometheus

Conversation

@tylergmuir
Copy link

First, I ran go mod tidy to correct the influxdb1-client being shows as an indirect module instead of being shown as a direct module.

Other than that, I made some changes to the prometheus code:

  • Moved the homepage content to a constant just to make it easier to read.
  • Got rid of the manual creation of the json and moved to using a map and json encoding to convert it to the byte array. This should make future changes to the json structure less error-prone.
  • I added a label called instance with the cluster name. The main reason is that the label cluster is frequently used when running prometheus in kubernetes to denote which kubernetes cluster prometheus is running in. Since this is used for deduplication of metrics, it overrides the cluster label in the metrics resulting in nothing other than the endpoint name and port to identify which isilon cluster is which. Note that the instance label will come across as exported_instance. I also left the cluster label for backwards compatibility.

@tenortim
Copy link
Owner

Ugh, so sorry I missed this. Just getting round to catching up and working on the collector again.
I've made breaking changes so I can't merge directly. If you'd be willing to rework them, that would be great, otherwise, I will look to incorporate them and acknowledge you as the author.
Wrt the labels, if you're just adding an additional one, that is probably fine as is, but I wonder if we should leave the default behavior alone and make adding an additional label optional and tied to a config setting for the prometheus provider?

@tylergmuir
Copy link
Author

@tenortim you can go ahead and implement the changes within your code. It looks like some of the modifications I have made in my fork made irrelevant anyway.

As for the label, it may be good to a config option for like "cluster_label_name" or something like that where the user could configure the desired label name that is used. In a perfect world, I think "cluster" makes the most sense in terms of the Isilon/PowerScale metrics. The only issue is it is frequently used with the replica label for deduping data from multiple Prometheus instances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants