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.

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

Add counter for admissions #6087

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dweber019
Copy link
Contributor

What this PR does / why we need it:

Adding a counter to observe admissions.
Additionally, I moved the creation of the metrics store to share the instance.

Which issue this PR fixes:

Fixes #6084

Special notes for your reviewer:

Pretty new to golang and this project, feedback welcome 😉

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@dweber019 dweber019 requested a review from a team as a code owner May 26, 2024 00:06
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

This looks solid 👍

1 thing that this is missing is adding this metric in the list of expected metrics in

wantMetrics := []string{
metrics.MetricNameConfigPushCount,
metrics.MetricNameConfigPushBrokenResources,
metrics.MetricNameTranslationCount,
metrics.MetricNameTranslationBrokenResources,
metrics.MetricNameConfigPushDuration,
metrics.MetricNameConfigPushSuccessTime,
}

Additionally: would you also be interested in contributing this new metric description to docs https://docs.konghq.com/kubernetes-ingress-controller/latest/production/observability/prometheus/ ? The source is located at: https://github.com/Kong/docs.konghq.com/blob/e3f336ea4f63c6808d9ab40ed7f522ef47ff27fc/app/_src/kubernetes-ingress-controller/production/observability/prometheus.md

@dweber019
Copy link
Contributor Author

Thx, having a look this evening.

@dweber019
Copy link
Contributor Author

Added changes and PR for documentation is open at Kong/docs.konghq.com#7429

internal/manager/setup.go Outdated Show resolved Hide resolved
internal/manager/run.go Outdated Show resolved Hide resolved
internal/admission/handler.go Show resolved Hide resolved
internal/admission/handler.go Outdated Show resolved Hide resolved
internal/admission/handler.go Outdated Show resolved Hide resolved
@pmalek
Copy link
Member

pmalek commented May 31, 2024

@dweber019 FYI: our CI configuration currently doesn't allow contributions from forks (ref: #3745). When that's resolved we'll be able to merge this PR.

@dweber019
Copy link
Contributor Author

So should I create a feature branch directly on this repo?

@pmalek
Copy link
Member

pmalek commented May 31, 2024

branch

You won't be able to unless you're a contributor/org member with write access. Sorry for any inconvenience caused.

@dweber019
Copy link
Contributor Author

OK so I understand that I wait for #3745. Hope get's resolved soon 😉

randmonkey
randmonkey previously approved these changes Jun 3, 2024
Signed-off-by: David Weber <[email protected]>
Signed-off-by: David Weber <[email protected]>
@pmalek
Copy link
Member

pmalek commented Nov 4, 2024

OK so I understand that I wait for #3745. Hope get's resolved soon 😉

@dweber019 With #3745 merged now, you can try rebasing on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admission webhook observability
3 participants