fabio icon indicating copy to clipboard operation
fabio copied to clipboard

Add additional clean function for prometheus metric names

Open corest opened this issue 1 year ago • 5 comments

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

corest avatar Aug 24 '22 21:08 corest

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 24 '22 21:08 CLAassistant

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.

nathanejohnson avatar Aug 26 '22 01:08 nathanejohnson

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?

corest avatar Sep 05 '22 10:09 corest

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 avatar Sep 06 '22 02:09 nathanejohnson

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

corest avatar Nov 07 '22 22:11 corest