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 additional clean function for prometheus metric names #899

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

Conversation

corest
Copy link

@corest corest commented Aug 24, 2022

Issue description

With hostnames having dash in name and fabio using prometheus metrics, I'm running into the issue like:

2022/08/24 21:21:47 [INFO] Registering metrics provider "prometheus"
panic: descriptor Desc{fqName: "vol-client-1_fabio_route", help: "", constLabels: {}, variableLabels: [service host path target]} is invalid: "vol-client-1_fabio_route" is not a valid metric name

goroutine 1 [running]:
github.com/prometheus/client_golang/prometheus.(*Registry).MustRegister(0xc00

It is indeed invalid metric name as there is - in metric name which is not allowed.

With this PR there is another function added alongside clean called clean_prom and used in prometheus provider.
Other option would be changing clean function directly but that can break the existing environment with the upgrade.
Therefore, adding additional function is safer.
If this approach is fine - I'll update documentation as well, but first want to get feedback if I'm missing something or this approach is too simple to solve the issue

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2022

CLA assistant check
All committers have signed the CLA.

@nathanejohnson
Copy link
Member

nathanejohnson commented Aug 26, 2022

So overall I like the idea, but one thing that might be an improvement would be to handle the case where a hostname starts with a numeric character, as was encountered here:

#878

Ideally we'd have a prefix in front of the hostname as a default, like maybe fabio_, but this default metrics prefix has been the same since metrics were introduced and I don't want to break backwards compatibility.

@corest
Copy link
Author

corest commented Sep 5, 2022

The way I handled it for my issue is by fixing this prefix with templating.
I'm using Fabio with Nomad and Consul, so for the Fabio config I just added

      template {
        data = <<EOF
[[- $hostname := env "HOSTNAME" ]]
...
metrics.target=prometheus
metrics.prefix = [[ $hostname | replaceAll "-" "_" ]]_{{ `{{clean .Exec}}` }}
EOF
        change_mode = "restart"
        destination = "local/fabio.cfg"
        left_delimiter = "[["
        right_delimiter = "]]"
      }

@nathanejohnson so you say we should leave it as it is and allow users to handle this issue by explicitly adjusting metrics.prefix?

@nathanejohnson
Copy link
Member

that is certainly an option. Regardless, since this has come up twice now, the docs could be clearer that metrics.prefix is still in play with prometheus, and maybe make mention of how picky prometheus is. I will take that as as to-do for myself. But I do like your clean method as well.

@corest
Copy link
Author

corest commented Nov 7, 2022

@nathanejohnson I somehow forgot about this. So do you think this still makes sense or just docs update enough?

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