fabio
fabio copied to clipboard
Add additional clean function for prometheus metric names
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
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.
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
?
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.
@nathanejohnson I somehow forgot about this. So do you think this still makes sense or just docs update enough?