-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metrics: Add --metrics-per-undefined-host
argument.
#11818
Metrics: Add --metrics-per-undefined-host
argument.
#11818
Conversation
Hi @grounded042. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-ingress-nginx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/cc @strongjz |
I meant to look at this Sunday. I will tonight. |
--metrics-per-undefined-host
.
--metrics-per-undefined-host
.--metrics-per-undefined-host
argument.
…t defined in ingresses Signed-off-by: Jon Carl <[email protected]>
Signed-off-by: Jon Carl <[email protected]>
Signed-off-by: Jon Carl <[email protected]>
4fd33ee
to
f872474
Compare
I've rebased the branch so it's ready to merge. |
/kind feature |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grounded042, strongjz 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR fixes #3844 by adding a flag,
--metrics-per-undefined-host
whichallows host labels on request metrics even if the host is not defined on an
ingress.
Today when a request metric comes in it will be dropped if the host label is
not a host explicitly defined on an ingress which ingress-nginx is tracking.
Even if
--metrics-per-host
is set totrue
it would drop metrics. You couldset
--metrics-per-host
tofalse
, but then thehost
label was not includedin the metrics. The reasoning for this was due to wildcard hosts. Someone could
send many requests with different hostnames not defined on any ingress and the
cardinality of request metrics would explode due to the many hosts. This PR
gives those who accept the cardinality explosion risk a way to get metrics with
the host label included even when the host is not defined on an ingress.
Types of changes
Which issue/s this PR fixes
Fixes #3844
How Has This Been Tested?
I have tested this in multiple ways:
make dev-env
kind cluster. I ran the code beforeand after my change and checked metrics to see the metrics dropped before
and not dropped after.
Checklist: