Make /metrics listen on a different address

[reynico]

• read the CONTRIBUTING guidelines
• raised a GitHub issue or discussed it on the projects chat beforehand
• added unit tests
• added integration tests
• updated documentation if needed
• updated CHANGELOG.md

Addresses #343

[kradalby]

It seems a bit excessive to allocate a different port for this, limiting a network would seem sufficient? This would break peoples current setup.

[reynico]

limiting a network would seem sufficient?

not sure if there's any way to do it per URI. I've found https://github.com/bu/gin-access-limit but it needs to be attached to the whole Gin engine.

This would break peoples current setup.

I could add a switch so if there's no metrics_listen_addr it will use listen_addr so it'll be backward compatible. What do you think?

[kradalby]

Sorry for the late reply, I have not had time to make a decision here, I do not really care for the behaviour of having a flat set and not set.

It might be ok to have a separate listen, but it will have to wait a couple of releases, I'll get back to you when we have finished other stuff that are a bit pressing.

[ohdearaugustin]

I would maybe also add a option to enable/disable the metrics endpoint. Furthermore as it is a prometheus exporter I would suggest to use a port in the default port range of prometheus exporters. default port allocations

Maybe something between 9914 and 9921

[reynico]

@ohdearaugustin done! I picked 9915 as the Prometheus port and also added a switch to toggle it.

btw, seems like some of the tests on upstream are broken.

[kradalby]

Hi, sorry for the back and forth:

I think I have landed on that having a separate port is fine, just list it under the breaking change in the Changelog.

On the port: this is not an exporter, so use 8001, 9090 or other common web port.

The ports in the linked list is to be reserved for applications specifically designed for translating api to metrics.

[kradalby]

So remove the enable flag, and set default listen to local:9090.

[ohdearaugustin]

@kradalby Those metrics exported by gin are prometheus metrics. So I don't understand the argumentation that this is not an exporter. It is an exporter integrated in the application. For example Gitlab does the same with it's "exporters" they are also integrated into the application itself.

The ports in the linked list is to be reserved for applications specifically designed for translating api to metrics.

Never heard of that limitation beforehand. And furthermore why would you remove the ability to enable/disable the endpoint?

[kradalby]

Those metrics exported by gin are prometheus metrics. So I don't understand the argumentation that this is not an exporter.

And exporter is a component translating information that is not Prometheus compatible to Prometheus metrics[1]. For example, by querying an API and exporting Metrics.

What we do is instrument the code directly, and not rely on a thirdparty/microservice outside of headscale, and it is by the quote posted not an exporter:

The ports in the linked list is to be reserved for applications specifically designed for translating api to metrics.

The "correct" or optimal way to use prometheus, is for all applications in the world to just have a /metrics endpoint, making exporters unnecessary. We have built it in, but that page of ports is for apps that yet not have built-ins, e.g. Postgres. or where it makes sense to keep it outside from a performance perspective.

It is an exporter integrated in the application. For example Gitlab does the same with it's "exporters" they are also integrated into the application itself.

Basically GitLab is an example breaking the rule.

Never heard of that limitation beforehand. And furthermore why would you remove the ability to enable/disable the endpoint?

Metrics should always be available and it also serves as a health endpoint.

1. https://prometheus.io/docs/instrumenting/exporters/ describes a bit the difference of exporter and instrumentations.

[ohdearaugustin]

@kradalby Thank you for that information and you convinced me. I was wrong.
@reynico Sorry for also confusing you.

[kradalby]

No problem ;) Most of my days are spent in Prometheus land, so I got this into my head now :P

[reynico]

No prob! I'll revert the last two commits, plus set the listen port to 9090.

[kradalby]

Looks sensible, could you add an entry to the CHANGELOG under breaking?

[reynico]

Done @kradalby ! Want me to add something at features/changes as well?

[kradalby]

That should cover it, I'll merge it when tests pass