fabiolb / fabio

Consul Load-Balancing made simple
https://fabiolb.net
MIT License
7.25k stars 620 forks source link

Add additional clean function for prometheus metric names #899

Open corest opened 1 year ago

corest commented 1 year ago

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 commented 1 year ago

CLA assistant check
All committers have signed the CLA.

nathanejohnson commented 1 year ago

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:

https://github.com/fabiolb/fabio/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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