Prometheus metrics are redundant and slow

[renbou]

While digging into Caddy's source code I've noticed that every (!) route handler is wrapped in a metricsInstrumentedHandler which updates Prometheus metrics during request execution. While it is a great feature and should definitely be enabled by default in Caddy, it currently uses up way to much CPU time and the metrics provided are quite redundant.

Since Caddy tries out every route handler in order until it gets an answer to the request, metric instrumentation is called for each handler, even if it didn't actually partake (which is also quite difficult to define in this context) in resolving the request, so handler-specific metrics are constantly updated with unrelated data and as a result pretty much all of the handler-specific metrics are meaningless, making them only usable to track server-wide stats.

As an example, here are the metrics for a simple Caddy server with 2 hosts, 1 of which only has a reverse_proxy handler, and the other has 2 respond handlers, 1 reverse_proxy handler and 1 file_server handler. The metrics were taken after running an http load-testing tool on the endpoints.
prometheus-metrics.txt
As seen in the example, all of the handler-specific metrics are pretty much the same for all handlers, even though in reality only the respond handler was requested.

The handler metrics provide even less use if the web server hosts multiple domains, since requests from all domains get mixed up in the metrics.
However, questionable metrics wouldn't be much of an issue if they were as fast as providing server-wide metrics, but they, of course, aren't, since they are getting updated multiple times until the request is finally answered.

I've ran pprof while putting load using h2load to request one of the simple respond handlers, and it turned out that 73% of the time spent during caddyhttp.Server.primaryHandlerChain.ServeHTTP was in the metricsInstrumentedHandler (only 30% of the time was spend by the actual caddyhttp.StaticResponse.ServeHTTP). Here's the profile:
profile.zip

I really think that metrics such as these should be server-wide where they make more sense and are quicker. https://github.com/nginxinc/nginx-prometheus-exporter could be seen as an example of similar Prometheus metrics commonly used for nginx.

[renbou]

I've looked through the other issues regarding Prometheus metrics and if host-based metrics might some day be implemented, they shouldn't affect performance that much. As for adding support for customizable metrics - seems like they would be an even bigger problem performance-wise

[hairyhenderson]

Thanks for filing this @renbou - I'll try to find some time this coming week to dig into it. My gut feeling is that some of the issue is due to the way middleware is handled and nested by Caddy, so it might be a bit more tricky to fully fix.

On the surface this is alarming:

[...] that 73% of the time spent during caddyhttp.Server.primaryHandlerChain.ServeHTTP was in the metricsInstrumentedHandler (only 30% of the time was spend by the actual caddyhttp.StaticResponse.ServeHTTP).

However percentages can sometimes be misleading, so it would be good to get some actual numbers... I haven't looked at the profile yet (and I won't this weekend - recovering from jet lag).

Caddy's handling of static responses in particular is very fast, and so it's not actually that surprising that the instrumentation surrounding it would account for more CPU time. As with all performance tuning, the most useful question isn't "how fast can we make it go?" but instead "how fast does it need to be?" In SRE terms that might translate to "we have a latency SLO of 100ms 99% of the time, but we can't reach that reliably if the metrics instrumentation takes 80ms"

[renbou]

I might've solved how this and the other Prometheus issues/requests could be implemented/solved. As discussed previously in #4016, a new metrics component would need to be added to the server structure of the http app, where the wanted labels can be described somewhat like this in the Caddyfile:

metrics {
	label host {http.request.host}
	label content_type {http.response.header.Content-Type}
	label static_label "same value for everyone!"
}

The problem which surfaced previously is that we'd metrics would supposedly have to be lost on reloads. To solve this, we can globally store the currently existing prometheus.Collectors without labels (counters, GoCollector, etc) and prometheus.ObserverVecs (histograms), which are the entities performing actual metric collection.

When provisioning a server, we'll need to setup a new prometheus.Registry with the required metrics, which are:

1. Global metrics which have not been removed
2. New metrics
   as well as add the new metrics to the global ones (for next reloads). This way we aren't losing any metrics which haven't been changed, and the changed metrics will get lost during if we change labels anyway, since there isn't a clear way to migrate them.

When removing the old Metrics component during a reload, all we need to do is remove the "deprecated" metrics from the global store. To do so we can store a view on the metrics which were added to the current server's prometheus.Registry, which would then allow us to find the deprecated metrics by "xoring" the old server's metrics and the new global metrics.

To solve another set of issues including performance, needing information from the matchers (for example the host-matcher), we can simply leverage the already present replacer. Since, as mentioned in the beginning, labels should be configurable with placeholders, we could simply add the necessary values to the request replacer during any time of the request. For example, a host matcher could set the value for "http.server.host", the handler instrumentation (which is what currently affects performance by running the same metric instrumentation code for each route) could set the value for "http.server.handler", etc. All of the keys accumulated during the request could then be used during metric collection, which would happen one time and be one of the first handlers in the middleware chain (thus giving it access to ALL of the gathered placeholders), by simply getting them from the replacer.

The way of gathering metrics I've described here will also leave us with actually meaningful metrics about handlers (since the http.server.handler key will be set only once - after the handler which actually gave a response returns) and everything else, since they are gathered once per request instead of multiple times like they are now (which is why currently metrics are pretty useless).

[francislavoie]

Worth noting that metrics is already a handler directive to serve the actual metrics exposition endpoint (see https://caddyserver.com/docs/caddyfile/directives/metrics), so unfortunately I think we'll need a different config for it. We could have a labels directive maybe:

labels [<field> <value>] {
	<field> <value>
}

I'm thinking we might want to support somekind of setup where you could say when to evaluate the labels, i.e. if you want to add a label at the start of the request vs at the end (after all the next middlewares ran). Could be done with > and < prefixes on the field, so like >host and <content-type or something to that effect (as in, > for "on the way in" and < for "on the way out"). But idk if this makes sense?

How would your described changes behave with existing metrics collected? My understanding was that the histograms etc may need to be reset on config reload or something if their config changed? I'm not sure I totally follow. @hairyhenderson should definitely chime in here.

[renbou]

Currently metrics is a handler field in the JSON structure and the Caddyfile, however I'm talking about adding another metrics to the http.server in thr JSON structure (would be global in Caddyfile). I'm not sure if this will be ok with how the Caddyfile is parsed though.

As for when the metrics are collected what I meant was that the metrics handler would now be initialized only once as the first middleware in the chain, meaning that it'd start collecting metrics just before the request is handled and finish collecting them after, as well as use the replacer afterwards since it's filled with all of the needed placeholders at that point.

Existing metrics are reset only if their labels change, pretty much. If the labels change, then their isn't a proper way to "migrate" metrics with the old label space to the new one, because they act like totally different metrics. For example, when adding a host label, it wouldn't make sense to "continue using the old metric" and just append a new label to it - the results wouldn't make sense (also this is what can't be done using the prometheus library).

Actually, since currently Caddy's metrics to which we'd be adding the possibilities of label modification (pretty much request-specific histogram metrics) all depend on the same set of labels, we'd only need to keep a version of the labels which were used to provision the current server and the current labels as a global value, instead of having to upkeep multiple lists (Collectors, Observers...)

[francislavoie]

http.servers JSON options would go in the Caddyfile's servers global options: https://caddyserver.com/docs/caddyfile/options#server-options

I'm still not sure I totally follow, but that's probably just my fault for not having looked into it enough to wrap my head around it. But it sounds like more of a plan than we've had before, so that's exciting!

[renbou]

http.servers JSON options would go in the Caddyfile's servers global options: https://caddyserver.com/docs/caddyfile/options#server-options

Hmm, not all of them do - take http_port and https_port for example. Metrics can also be made global in the Caddyfile because it'd make more sense, I think.

This shouldn't be that hard to implement, but I'd like to contribute a few optimization-related changes since they should overall clean up the request-handling code a bit. But basically the way we won't be losing metrics is by using a new prometheus.Registry for each server (that actually collects the metrics and bundles them up for the http endpoint) but reuse the actual metrics between server runs (if the labels for those haven't changed, meaning that critical metrics would never be lost because they don't depend on that set of labels)

[francislavoie]

Hmm, not all of them do - take http_port and https_port for example

Those two options are on the http app, not on http.servers.

Metrics can also be made global in the Caddyfile because it'd make more sense, I think.

Well, it depends whether it's config per server or config on the http app, and it sounds like you're suggesting per-server. Wouldn't it be possible/make sense to have different label config per server?

[francislavoie]

Do you have an idea how #4140 could fit in maybe?

[renbou]

You're right, those options are on the http app. And the metrics config should definitely be a per-server thing (http.servers in the json), but in a Caddyfile we have no way of specifying multiple servers, which is why it should probably be a part of the global options.

As for the reverse proxy upstreams - I really do not see the point in adding latency and status code metrics for upstreams, since that is already available through the present metrics. A health status metric can surely be added though, and per-path configuration should be possible with what I've described here: we could add a "metrics" structure to the routes (http.servers.routes[i]) which could be a metric inclusion/exclusion list for the route.

Considering the fact that a lot of metrics might be added over the time, we can also make the server-wide metrics config look like this:

metrics {
  labels {
    host {http.server.host}
  }
  include {
    reverse_proxy {
      upstream {http.handlers.reverse_proxy.upstream}
    }
  }
  exclude {
    request_size_bytes
  }
}

which would allow enabling/disabling various metrics globally. Though we'd definitely have to think this design through a bit more, lol, since this doesn't look very nice.