add metric for max_conns of upstream servers #201
Conversation
lucacome
added
the
enhancement
collector/nginx_plus.go
Outdated
| @@ -292,6 +293,7 @@ func NewNginxPlusCollector(nginxClient *plusclient.NginxClient, namespace string | |||
| streamUpstreamServerMetrics: map[string]*prometheus.Desc{ | |||
| "state": newStreamUpstreamServerMetric(namespace, "state", "Current state", streamUpstreamServerVariableLabelNames, constLabels), | |||
| "active": newStreamUpstreamServerMetric(namespace, "active", "Active connections", streamUpstreamServerVariableLabelNames, constLabels), | |||
| "limit": newStreamUpstreamServerMetric(namespace, "limit", "Limit for connections", streamUpstreamServerVariableLabelNames, constLabels), | |||
There was a problem hiding this comment.
Hi @StevenHessing Thanks for the PR!
Could you possibly make the following changes for consistency? (here and for the stream metrics and the doc page) In the other metrics, we basically reuse the names that are in the NGINX Plus API, so the new metric stands out.
(the code will also need to be reformatted after applying the suggestion)
| "limit": newStreamUpstreamServerMetric(namespace, "limit", "Limit for connections", streamUpstreamServerVariableLabelNames, constLabels), | |
| "max_conns": newStreamUpstreamServerMetric(namespace, "max_conns", "Max_conns limit for connections", streamUpstreamServerVariableLabelNames, constLabels), |
There was a problem hiding this comment.
I have considered that. I settled on limit because that is what is shown in the Nginx+ dashboard and allows users to correlate this metric with what they see in the dashboard. The max_conns however is what is in the API and in the nginx upstream configuration. Please confirm what you prefer and I'll make any change necessary.
There was a problem hiding this comment.
That makes sense.
However, let's make sure we document that the limit still reflects the max_conns parameter. Additionally, better to mention that 0 means no limit.
Could you update the doc string to the following?
"Limit for connections which corresponds to the max_conns parameter of the upstream server. Zero value means there is no limit"
There was a problem hiding this comment.
I have updated the docstrings
Here is a Grafana formula we use for our systems:
topk(8,sum by (upstream) (nginxplus_upstream_server_active{env="$environment", site="$site"}) / sum by (upstream) (nginxplus_upstream_server_limit{env="$environment", site="$site"}))
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
|
Merging this, thanks @StevenHessing ! |
Proposed changes
This adds a metric called 'limit' for upstream HTTP and TCP/UDP servers representing the max_conns parameter for the upstream server.
Checklist
Before creating a PR, run through this checklist and mark each as complete.