Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feature: added AdmissionController metrics #7711

Merged
merged 3 commits into from Nov 2, 2021
Merged

feature: added AdmissionController metrics #7711

merged 3 commits into from Nov 2, 2021

Conversation

fblgit
Copy link
Contributor

@fblgit fblgit commented Sep 27, 2021

What this PR does / why we need it:

Adds AdmissionController basic instrumentation to show:

  • Rendered Ingresses Timings (measures the time required to compose the config objects)
  • Tested Ingresses Timings (measures the time required to test the ingresses)
  • Count Rendered Ingresses (measures how many Ingresses are present)
  • Count Tested Ingresses (measures how many ingresses are tested)
  • Configuration Size (measures the size in bytes of the config file)
  • Admission Roundtrip Timings (measures the whole Admission roundtrip)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

  1. Created UnitTest for this new AdmissionController Metrics
  2. Verified that metrics are being displayed on the controller output
  3. Verified that metrics are being listed on the metrics endpoint

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 27, 2021
@k8s-ci-robot
Copy link
Contributor

@fblgit: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @fblgit. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-priority cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2021
@fblgit
Copy link
Contributor Author

fblgit commented Sep 27, 2021

@rikatz first basic instrumentation for the admission controller as promised. Do let me know if there is anything else missing for this one.

tao12345666333
tao12345666333 reviewed Oct 3, 2021
View reviewed changes
Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign

@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 12, 2021
rikatz
rikatz reviewed Oct 12, 2021
View reviewed changes
internal/ingress/metric/main.go Outdated
@@ -130,6 +136,7 @@ func (c *collector) RemoveMetrics(ingresses, hosts []string) {
func (c *collector) Start() {
c.registry.MustRegister(c.nginxStatus)
c.registry.MustRegister(c.nginxProcess)
c.registry.MustRegister(c.admissionController)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the sake of saving a bit of CPU, can we check if Admission is configured before registering?

you can do something like

if n.cfg.ValidationWebhook != ""

somewhere, just so you can just register and start the admission controller

Or you can verify this earlier, like in line 96 and then check here if c.admissionController != nil :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sounds fair. let me fix these 3 things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the object is too large, just passed the string of the ValidationWebhook for this use-case into Start/Stop

@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2021

@fblgit PR looks great.

Left some comment just so we just collect the metrics if the validationWebhook is enabled (we can save just tiny CPU, but it still is CPU) and also a nit about the header

Thanks!

@rikatz rikatz added this to the v1.0.5 milestone Oct 12, 2021
@rikatz
Copy link
Contributor

rikatz commented Oct 24, 2021

/assign

@fblgit
Copy link
Contributor Author

fblgit commented Oct 28, 2021

@rikatz all yours, should be good now. sorry for the delay, had a hell of a week.

thank you

@rikatz
Copy link
Contributor

rikatz commented Nov 2, 2021

/label tide/merge-method-squash
/lgtm
/approve
Thanks!

@k8s-ci-robot k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 2, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fblgit, rikatz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2021
@k8s-ci-robot k8s-ci-robot merged commit a5bab6a into kubernetes:main Nov 2, 2021
kskalski pushed a commit to kskalski/ingress-nginx that referenced this pull request Nov 28, 2021
@fblgit @kskalski
feature: added AdmissionController metrics (kubernetes#7711)
709221f 
* feature: added AdmissionController metrics

* fix: flag control on admissionCollector

* fix: admission collector disclaimer year and linting
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
@fblgit @rchshld
feature: added AdmissionController metrics (kubernetes#7711)
3316f80 
* feature: added AdmissionController metrics

* fix: flag control on admissionCollector

* fix: admission collector disclaimer year and linting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tao12345666333 tao12345666333 tao12345666333 left review comments

@rikatz rikatz rikatz left review comments

@strongjz strongjz Awaiting requested review from strongjz

Assignees

@tao12345666333 tao12345666333

@rikatz rikatz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.0.5
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fblgit @k8s-ci-robot @rikatz @tao12345666333