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

⚠️ Remove global metrics registration in init, add registration func #2240

Closed
wants to merge 1 commit into from
Closed

⚠️ Remove global metrics registration in init, add registration func #2240

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion pkg/metrics/workqueue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,18 @@ func init() {
Registry.MustRegister(unfinished)
Registry.MustRegister(longestRunningProcessor)
Registry.MustRegister(retries)
}

// SetDefaultGlobalWorkqueueMetricsProvider registers the GlobalWorkqueueMetricsProvider
// as the global workqueue.MetricsProvider.
func SetDefaultGlobalWorkqueueMetricsProvider() {
workqueue.SetProvider(GlobalWorkqueueMetricsProvider())
}

workqueue.SetProvider(workqueueMetricsProvider{})
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the k/k pr, we wont merge this. This breaks the majority use-case (workqueue metrics just work without doing anything) in order to support a minority use-case (set custom provider).

/hold

Copy link
Author

Choose a reason for hiding this comment

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

Is this not what you were suggesting here? kubernetes/kubernetes#114242 (comment)

This is marked as a breaking change, and allows consumers to set it. Unsure what the path forward is if it is neither of the options I've proposed.

Copy link
Member

Choose a reason for hiding this comment

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

No, my suggestion was to change the upstream workqueue to not use a sync.Once so what is set by controller-runtime can be overridden later if desired.

Copy link
Author

@austince austince Mar 14, 2023 •
edited

Choose a reason for hiding this comment

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

Alright, I'll send a PR for that -- seems to paper over the fact that this library is mutating global state and the "metrics just work without doing anything" use case could be addressed somewhere else (like kubebuilder, which sets up a main), but if this gets it in 🤷🏼

I'll leave this open in the meantime if others want to weigh in.

Copy link
Member

@sbueringer sbueringer Mar 15, 2023 •
edited

Choose a reason for hiding this comment

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

I think changing kubebuilder does not really mitigate the situation. I don't have a good overview over the ecosystem but the controllers that I'm aware of don't regularly regenerate their main file with kubebuilder.

Copy link
Author

Choose a reason for hiding this comment

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

Okie, I'll close this then -- I've also got a PR for what Alvaro suggested here, if you guys want to drive that approach: kubernetes/kubernetes#116616

// GlobalWorkqueueMetricsProvider constructs a workqueue.MetricsProvider
// that registers metrics in the global Registry.
func GlobalWorkqueueMetricsProvider() workqueue.MetricsProvider {
return workqueueMetricsProvider{}
}

type workqueueMetricsProvider struct{}
Expand Down