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

Jump to bottom

Add metric to count number of sync calls #1692

Merged
merged 4 commits into from Jun 5, 2019
Merged

Add metric to count number of sync calls #1692

merged 4 commits into from Jun 5, 2019

Conversation

cheukwing
Copy link
Contributor

What this PR does / why we need it:
Adds a Prometheus metric to count the number of Sync() calls for each controller

Which issue this PR fixes: fixes #1001

Special notes for your reviewer:
Each controller struct holds a metrics field, following the existing field in the certificates controller struct.
However since Default can be accessed globally and is the same as each of these metrics fields, I am not sure if it is actually needed.

Release note:

Adds `ControllerSyncCallCount` prometheus metric to count sync calls from each controller

@cheukwing
Add ControllerSyncCallCount metric
f2e57a1 
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. release-note Denotes a PR that will be considered when it comes time to generate release notes. area/acme Indicates a PR directly modifies the ACME Issuer code labels May 17, 2019
@jetstack-bot jetstack-bot requested a review from munnerz May 17, 2019 13:24
@jetstack-bot jetstack-bot added area/monitoring Indicates a PR or issue relates to monitoring size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 17, 2019
@cheukwing
Bazel update scripts
840cba3 
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
@cheukwing
Use ControllerName as label value
455f6eb 
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
munnerz
munnerz reviewed May 19, 2019
View reviewed changes
pkg/controller/acmechallenges/controller.go Outdated
@@ -63,6 +64,7 @@ type Controller struct {

watchedInformers []cache.InformerSynced
queue workqueue.RateLimitingInterface
metrics *metrics.Metrics
Copy link
Member

Choose a reason for hiding this comment

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

Each controller shouldn't need its own pointer to metrics, and if it does then I think they should all point to the same actual instantiation 😄

Copy link
Contributor Author

@cheukwing cheukwing May 20, 2019 •
edited

Choose a reason for hiding this comment

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

I have removed them all to just call with the global metrics.Default, except for the certificates controller which already had a metrics pointer. However I'm not sure if this controller needs this anyway since this is just an alias for the same metrics.Default.

Additionally I don't quite understand the need for the Prometheus metrics to be exposed globally themselves in the metrics.go file since they can be accessed via the global metrics.Default anyway, unless this is a requirement for Prometheus.

@cheukwing
Remove metrics field from controllers
7d67ae4 
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
@jacobstr jacobstr mentioned this pull request May 24, 2019
Open
@munnerz
Copy link
Member

munnerz commented Jun 3, 2019

Looks good! 😄

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2019
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheukwing, munnerz

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2019
@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

11 similar comments
@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

13 similar comments
@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@jetstack-bot jetstack-bot merged commit edd2225 into cert-manager:master Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@munnerz munnerz munnerz left review comments

Assignees

@munnerz munnerz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/acme Indicates a PR directly modifies the ACME Issuer code area/monitoring Indicates a PR or issue relates to monitoring dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a prometheus metric for number of times a sync function is called
4 participants
@cheukwing @munnerz @jetstack-bot @retest-bot