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

Add new metrics to exporter, and add name labels to every metric #154

Closed
wants to merge 1 commit into from

Conversation

josebrutalsys
Copy link

Additionally, it adds docker-compose to be able to run the environment more easily from local.

@znerol
Copy link
Member

znerol commented Jul 13, 2023

Thanks for taking the time to file a PR. Regrettably this is mixing several unrelated feature requests. Hence it cannot be merged as is.

I generally recommend to open an issue for a feature request first so as to give people an opportunity to discuss viability and approach.

That said:

  • Deployment recipes should go in the wiki (docker compose).
  • Additional labels on resources will lead to duplicated time series in cluster setups. Read the comments in Collect from current node only information about node's VMs/CTs/etc #54 and the linked blog posts for hints on how to join labels on pve_guest_info or pve_storage_info to the metrics.
  • I personally will not merge anything which touches ClusterNodeConfigCollector or which wants to iterate through self._pve.nodes.get(). This has been a source of trouble for many users in the past. If there is somebody who wants to maintain that part and understands its quirks and also support people whose clusters are running into errors because of that, then please see Looking for a Co-maintainer #84.

Please don't be sad because of this rejection. I'm trying to answer/decide on PRs early instead of letting them float around eternally.

@znerol znerol closed this Jul 13, 2023
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.

3 participants