metrics: move metrics up, outside servers

[mohammed90]

This change moves the metrics configuration from per-server level to a single config knob within the http app. Enabling metrics in any of the configured servers inside http enables metrics for all servers.

Fix #6604

[aa-shalin]

any idea when this will be merged? I ran into this issue after i was advised to use the beta version of Caddy

[erikschul]

This looks wrong to me.

I argue that it's reasonable to want to enable a metrics route for a specific server (e.g. :9000), and not serve it on all servers (including public ones). Metrics generally aren't made publicly available.

[francislavoie]

This isn't about serving metrics, it's about enabling metrics. It doesn't really work to only enable metrics for one server because the Context it gets given is global, not a Context per server. (Server here is in the JSON config concept, i.e. :9000 is a separate server from :9001 but two domains on :443 are on the same server, see https://caddyserver.com/docs/caddyfile/options#server-options).

[erikschul]

Okay; if it's technically complicated, maybe a better approach is to keep the metrics internal to :2019/metrics and then use

route /metrics {
    reverse_proxy localhost:2019/metrics
}

wherever you want to expose it?

I think what confuses me is the line s.Metrics = nil.
because I would think that this means that the metrics are disabled for each server?
I'm not familiar with the codebase. Maybe I'm misreading it.
I thought this meant that
a) the individual server configuration is no longer possible (deleted)
b) the global configuration enables metrics globally, overwriting the setting with the last value of the iteration:

metrics = cmp.Or[*caddyhttp.Metrics](metrics, &caddyhttp.Metrics{})
			metrics = &caddyhttp.Metrics{
				PerHost: metrics.PerHost || s.Metrics.PerHost,
			}

[francislavoie]

I think you're confused. There's a metrics http handler and a metrics global option. The former is to serve the metrics (from any endpoint you want, none by default obviously, except for the admin endpoint) and the latter is to enable HTTP metrics. This PR is about moving the global option only.

[mohammed90]

Okay; if it's technically complicated, maybe a better approach is to keep the metrics internal to :2019/metrics and then use

route /metrics {
    reverse_proxy localhost:2019/metrics
}

wherever you want to expose it?

You're confusing 2 things: enabling HTTP handling metrics collection and exposure/serving on the admin endpoint, and exposing metrics on an endpoint other than the admin endpoint. Enabling metrics here (in this PR), means enabling the HTTP traffic metrics collection. They're exposed only on admin endpoint (default: :2019/metrics). To expose metrics on an endpoint other than the admin endpoint, you'd use the metrics handler.

Previously (before this PR), enabling HTTP metrics collection was done by adding the key metrics in the JSON inside a particular "server" (see the empty metrics object on this page: https://caddyserver.com/docs/json/apps/http/). Image for posterity:

This is misleading because metrics were always enabled for all servers, plus a bunch of other issues. The fix requires lifting the metrics object to be direct descendant of http (outside servers) because the collection of HTTP metrics is either enabled for all servers or none. server in this context means a logical group of listeners with unified behavioral pattern, not referring to Caddy as a whole. If you want HTTP metrics, it's enabled on the HTTP app, not "per server", so this PR is correct.

wherever you want to expose it?

Use the metrics handler for this.

I think what confuses me is the line s.Metrics = nil.
because I would think that this means that the metrics are disabled for each server?

That's not the correct interpretation, not that it was ever possible anyways. Nil-ing the field doesn't change the behavior. The line you're looking at is about adapting the old configuration to the new one. Once we copied the configuration, the older data is no longer meaningful because it's not used. We only need it there for existing users who haven't changed their config. We don't want to suddenly break them, so I added a translation.

a) the individual server configuration is no longer possible (deleted)

It never was. There's a label for the server name to track which "server" it's part of. As a caddyfile user, you'll rarely have to deal with the notion of server, unless you serve sites on multiple ports other than 80/443.

b) the global configuration enables metrics globally, overwriting the setting with the last value of the iteration:

Nothing is lost anyways. The field PerHost is only introduced in 2.9.0-beta. The lines you're looking at ensure that if the user still used the older field, and they happen to set per_host to true in any of their servers, the value is carried globally.

[erikschul]

Thanks @mohammed90 for the explanation! I'm new to the project but I hope I'll find ways to contribute as I get more involved. Thanks again!