Remove prometheus labels with high cardinality

[aledbf]

Replaces #2699

[aledbf]

ping @discordianfish

[discordianfish]

@aledbf Nice! LGTM, assuming that ngx.var.location_path is the path configured in the config (vs something coming from the client).

[aledbf]

@discordianfish please check #2702 . That PR just replaces the URI label with the fixed Path defined in the Ingress

[aledbf]

Nice! LGTM, assuming that ngx.var.location_path is the path configured in the config (vs something coming from the client).

Yes

[discordianfish]

@aledbf Why are there two new PRs now? This here looks like the way to go while #2702 still has the remote ip etc in it.

[aledbf]

Why are there two new PRs now? This here looks like the way to go while #2702 still has the remote ip etc in it.

Just to show an alternative to remove all the labels.

[discordianfish]

Ah got it. So yeah I'd go with this here since there is no way to ensure that the other vars are bounded.

[aledbf]

@discordianfish thank you!

[antoineco]

Why not location_path? Or the other way around, why location_path in other places and path here.

[aledbf]

location_path -> from nginx (variable)
path          -> go side

[antoineco]

Ah right, nginx_environment vs. expected_json_stats

[antoineco]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [aledbf,antoineco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov-io]

Codecov Report

Merging #2701 into master will increase coverage by 0.16%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2701      +/-   ##
==========================================
+ Coverage   40.92%   41.08%   +0.16%     
==========================================
  Files          72       72              
  Lines        5087     5084       -3     
==========================================
+ Hits         2082     2089       +7     
+ Misses       2716     2707       -9     
+ Partials      289      288       -1

Impacted Files	Coverage Δ
internal/ingress/metric/collector/collector.go	3.08% <0%> (+0.05%)	⬆️
cmd/nginx/main.go	27.73% <0%> (+5.1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0ed143...6c8647a. Read the comment docs.

[elafarge]

👌 Thanks a lot for the quickfix :)

[mbugeia]

Thanks for the quickfix but for me it's not totally fixed, we still have some labels with high cardinality.
upstream_ip and upstream_status can cause a lot of metrics to be created in particular when there are retry.
For example we can have upstream_status="504, 502, -", upstream_status="504, 200"... and in upstream_ip the order can change at each request (especially when you have a lot of backend).

In my case with only 2 ingress configured on the controller:

www-data@ingress-5ff44654b9-7hfg7:/etc/nginx$ curl -s localhost:10254/metrics |wc -l 
16965

[aledbf]

upstream_ip and upstream_status can cause a lot of metrics to be created in particular when there are retry.

But that's a good thing, right? This is telling you have some problems with the application or with the configured timeouts. In fact, you should create some alerts for that codes (504 and 502)

In my case with only 2 ingress configured on the controller:

How many endpoints do you have? Also, how many request for that number of events?

[mbugeia]

IMO there are 2 problems here:

• In case of retry the metrics are unusable because the label mix information from several different upstream requests. And filtering by a label like upstream_status="504, 502, -" means nothing for me.
• upstream_ip by its nature have a high cardinality. In my example we have between 3-5 endpoint depending scaling) but we have some service with 20 endpoints and can easily imagine other use cases with 100+ endpoints.

Moreover each of theses label involves a combinatorial explosion, it increase the number of metrics by adding combinaison of upstream_ip and upstream_status.
I think the number of metrics need to be limited, I'm glad that there are more precise metrics that in the vts module (like by accurate http status code) but too many harms the observability and I'm very concerned about breaking my prometheus instances.

If you don't want to drop entirely theses labels, to add the ability to exclude labels like in #2699 would be a good alternative.

[aledbf]

@discordianfish can you provide some feedback for ^^ ?

[aledbf]

upstream_status="504, 502, -" means nothing for me.

This means the first response from the upstream server returned 504, then 502 from the next one and finally - (usually means connection closed).

Can you post the log for this example? (ingress controller pod log).

[discordianfish]

upstream_ip by its nature have a high cardinality. In my example we have between 3-5 endpoint depending scaling) but we have some service with 20 endpoints and can easily imagine other use cases with 100+ endpoints.

This is fine IMO, it's bounded by the number of backend servers. Having hundreds seems reasonable.
You can always drop metrics on scape: https://www.robustperception.io/dropping-metrics-at-scrape-time-with-prometheus/

But the always changing upstream ips might become a problem for a long living ingress controller, not sure if the local registry every gets cleaned up. Usually you would use ConstMetrics that just return a value from another system (e.g vts before), so you don't need to keep state..

@brancz @SuperQ: Do you have some thought on this?

[mbugeia]

This means the first response from the upstream server returned 504, then 502 from the next one and finally - (usually means connection closed).

Sorry I misspoke myself, I know what it mean, but I think having a prometheus label like this has no meaning. What does a metric like upstream_response_time{upstream_status="504, 502, -"} mean ? Is it the response time of the first or the last request ? I can't imagine a use case where I want to use this.

I can't provide the log for now (no longer at work) but I don't think it will help, I know why the upstream didn't respond, I was running a load test and the upstream took too long to respond. What I could provide tomorrow if you want is an example of metrics created by my load test that illustrate my point.

This is fine IMO, it's bounded by the number of backend servers. Having hundreds seems reasonable.

As I understand it's not only bound by the number of backend servers but at least by:

• the number of backend servers
• the number of retry (retry will create label like upstream_ip="10.0.0.1, 10.0.0.2, 10.0.0.3" not in a predictable order, same with upstream_status but with http status code)

And this for each protocol, http status code, host and method. I dont know the exact maths but thing can really add up.

I'm not sure I'm explaining my point correctly. I will try to expand my testing tomorrow and provide some sample of the /metrics endpoint.

[aledbf]

What does a metric like upstream_response_time{upstream_status="504, 502, -"} mean ? Is it the response time of the first or the last request ?

You are right about this. The variables upstream_ip, upstream_response_time and upstream_status are strings comma separated so we can split and generated three metrics. Not sure if this makes sense dough, the list contains information about what was required to return a response not different client requests.

the number of retry (retry will create label like upstream_ip="10.0.0.1, 10.0.0.2, 10.0.0.3" not in a predictable order, same with upstream_status but with http status code)

I understand what you say but any alteration to the order could introduce a misunderstanding about what nginx is doing.

[aledbf]

I'm not sure I'm explaining my point correctly. I will try to expand my testing tomorrow and provide some sample of the /metrics endpoint.

Yes, this scenario makes sense, that's why I am asking for the log so we can see how to improve this

[brancz]

If I understand the overall architecture of this correctly, then the lua scripts on every request push metrics to the nginx-controller process via the unix socket, upon which the nginx-controller process increments/sets all metrics respective to that backend. This is going to get leaky if not handled carefully, I've seen numerous memory leaks due to leaking metrics (ironic, I know). Basically what you need to make sure, is that you only ever expose metrics of those ingress/backends/etc. that are actually in existance at a current moment in time. The strategy I would use here, albeit not particularly great is to keep increment/set logic as you have it and have another go routine, that regularly cleans up any metrics that do not have a respective ingress object associated with it (anymore). If I understand correctly this should not be a problem as the nginx-controller fully configures nginx so it will always know whenever any of these things change.

ConstMetrics as mentioned by @discordianfish are also an option, but it would mean you need to cache the metric data sent through the unix socket yourself, in which case you would find yourself doing roughly the same housekeeping, as you would still always need to make sure to throw away old data. The only positive would be that you could control not exposing stale metrics at scrape time, rather than by relying on data to be cleaned up properly.

[aledbf]

@brancz thank you for the feedback

The strategy I would use here, albeit not particularly great is to keep increment/set logic as you have it and have another go routine, that regularly cleans up any metrics that do not have a respective ingress object associated with it (anymore)

This should be easy to implement. We have a point in the code where we swap the "old" configuration with the new one where we can do the cleanup.

[brancz]

That sounds good. You will want to have a look at the Delete[0] and/or DeleteLabelValues[1]. Make sure to attempt to delete all metrics for all possible label combinations, for example, you will need to attempt to remove all metrics for all statuses and method.

[0] https://godoc.org/github.com/prometheus/client_golang/prometheus#CounterVec.Delete
[1] https://godoc.org/github.com/prometheus/client_golang/prometheus#CounterVec.DeleteLabelValues