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

Fix Prometheus node/cluster labels #839

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

jage
Copy link
Member

@jage jage commented Nov 12, 2024

WHAT is this pull request doing?

  • Add lavinmq_node label, hostname to differentiate nodes in cluster
  • Fix lavinmq_cluster to be a common cluster name for all nodes, this will allow Prometheus grouping by cluster

Cluster name must be set on all nodes, to the same value, via the clustering_name variable.

HOW can this pull request be tested?

Start LavinMQ with --clustering-name=my-cluster-name, scrape metrics.

$ curl -s http://guest:guest@localhost:15672/metrics | grep cluster
lavinmq_identity_info{lavinmq_version="2.0.0-9-g558bb98f", lavinmq_node="8E84C28F.local", lavinmq_cluster="my-cluster-name"} 1

Example from Prometheus TSDB:

Screenshot 2024-11-12 at 10 26 37

* lavinmq_node should be the hostname to differentiate nodes in cluster
* lavinmq_cluster should be a common cluster name for all nodes, this
  will allow Prometheus grouping by cluster

Cluster name must be set on all nodes, to the same value, via the
`clustering_name` variable.
@jage jage requested a review from a team as a code owner November 12, 2024 12:44
Copy link

github-actions bot commented Nov 12, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@jage
Copy link
Member Author

jage commented Nov 12, 2024

I have read the CLA Document and I hereby sign the CLA

lavinmq-user-84 added a commit to cloudamqp/CLA-signatures that referenced this pull request Nov 12, 2024
@jage
Copy link
Member Author

jage commented Nov 12, 2024

Happy to discuss other implementations, and if ok I can look at docs.

Copy link
Member

@kickster97 kickster97 left a comment

Choose a reason for hiding this comment

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

looks good :)
Minor: is cluster_name better than clustering_name?

@jage
Copy link
Member Author

jage commented Nov 12, 2024

looks good :) Minor: is cluster_name better than clustering_name?

Definitely more aesthetically pleasing but ... 🙂
... all other options are grouped under clustering, e.g. in the configuration file it's a section with [clustering] so now it's just name = ....

@kickster97
Copy link
Member

looks good :) Minor: is cluster_name better than clustering_name?

Definitely more aesthetically pleasing but ... 🙂

... all other options are grouped under clustering, e.g. in the configuration file it's a section with [clustering] so now it's just name = ....

Ah.. makes sense, oh well!

src/lavinmq/config.cr Outdated Show resolved Hide resolved
Co-authored-by: Carl Hörberg <carl@84codes.com>
@carlhoerberg
Copy link
Member

How can you group on it unless you add the label to all metrics? Doing some kind of transformation somewhere? If that's case, isn't it possible to deduct the (CloudAMQP) cluster name by just dropping the last 3 chars from the node name?

@jage
Copy link
Member Author

jage commented Nov 13, 2024

How can you group on it unless you add the label to all metrics? Doing some kind of transformation somewhere?

Exactly, you "join" the time series, example: lavinmq_queues * on(instance) group_left(lavinmq_cluster, lavinmq_node) lavinmq_identity_info to get the extra labels on the queues. Think it's pretty "standard", especially if you have high cardinality labels.

If that's case, isn't it possible to deduct the (CloudAMQP) cluster name by just dropping the last 3 chars from the node name?

Not sure that's possible in PromQL if I understand correctly, i.e. group by part of a label.

Relevant questions here:

  • Is the intent to have a lavinmq_cluster label? I assumed yes since it was there., and I think LavinMQ should have have this label to avoid relabelling during scrape.
  • Do you have any intent on ever having an entitiy "cluster name" for anything else then metrics? If not this could be implemented in other ways (--extra-metrics-labels or something like that)

@carlhoerberg
Copy link
Member

Relevant questions here:

  • Is the intent to have a lavinmq_cluster label? I assumed yes since it was there., and I think LavinMQ should have have this label to avoid relabelling during scrape.

  • Do you have any intent on ever having an entitiy "cluster name" for anything else then metrics? If not this could be implemented in other ways (--extra-metrics-labels or something like that)

Yeah, nothing comes to mind now how we could use this for anything else, hence my hesitation. The label makes sense, but --extra-metrics-label lavinmq_cluster=red-panda --extra-metrics-label foo=bar sounded like a more generic idea!

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.

4 participants