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 metrics for knowing adoption #122

Merged
merged 10 commits into from
Aug 17, 2021
Merged

Conversation

jyotimahapatra
Copy link
Contributor

@jyotimahapatra jyotimahapatra commented Aug 16, 2021

Issue #, if available:

Description of changes:
While handling the infrastructure for a cluster, we need a way to know which clusters are using the webhook to drive changes that change default values through the infra.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jyotimahapatra jyotimahapatra requested a review from a team as a code owner August 16, 2021 22:10
pkg/cache/cache.go Outdated Show resolved Hide resolved
pkg/cache/cache.go Outdated Show resolved Hide resolved
wongma7
wongma7 previously approved these changes Aug 16, 2021
Copy link
Member

@wongma7 wongma7 left a comment

Choose a reason for hiding this comment

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

one comment about metric name lgtm otherwise

pkg/handler/handler.go Outdated Show resolved Hide resolved
wongma7
wongma7 previously approved these changes Aug 16, 2021
wongma7
wongma7 previously approved these changes Aug 16, 2021
@jyotimahapatra jyotimahapatra changed the title add metrics for number of SA annotations being used add metrics for knowing adoption Aug 16, 2021
nckturner
nckturner previously approved these changes Aug 17, 2021
Copy link
Contributor

@nckturner nckturner left a comment

Choose a reason for hiding this comment

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

Would an equivalent metric that contains more information be a better alternative, like number of pods injected, number of service accounts with annotations seen, etc? If num_annotated_service_accounts_seen = 0 then we can assume the cluster is not using the webhook, but I'm not sure a good way to actually keep track of that. A set of service account UIDs would work but would grow unbounded. A gauge that returned the current number of annotated service accounts would work but wouldn't show clusters that had seen previous usage (not sure if we want that). Maybe you can add some context to the description, and if this is just the simplest option for the information we need, I'm ok with that.

}

// We need a way to know if the webhook is used in a cluster to drive changes.
// We could perform more interesting operations by knowing how many service accounts are being annotated.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this phrasing is a little awkward, maybe drop the "to drive changes"? Also what are you referring to regarding "more interesting operations"?

Copy link
Contributor Author

@jyotimahapatra jyotimahapatra Aug 17, 2021

Choose a reason for hiding this comment

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

I brainstormed a bit about this with @wongma7 . The number of pods injected is a good metrics, but that doesn't give us details about usage if the pods don't churn. We could add that as well, but we decided we don't have a usecase for the information.
The number of annotated service accounts is a good metric, but we it needs a bit of refactoring to make sure resync and update don't cause noise in the number. We chose to stick closer to what we need, know about usage in the clusters we operate.

@jyotimahapatra jyotimahapatra dismissed stale reviews from nckturner and wongma7 via 761947b August 17, 2021 17:02
@nckturner nckturner merged commit 66ce9fd into aws:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants