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

[stacked 1/5] metrics: cleanup metrics registration, collection and gathering. #403

Merged
merged 6 commits into from
Nov 20, 2024

Conversation

klihub
Copy link
Collaborator

@klihub klihub commented Nov 9, 2024

This patch series refactors our metrics collection code, moving most of the code to handle metrics registration, collection, and gathering into a single package in pkg/metrics. In particular, this series

  • consolidates all common metrics-handling code in pkg/metrics
  • removes metrics polling scattered around in resmgr, resmgr/{metrics,policycollector}
  • moves policy-specific metrics collection/polling to the policy package itself
  • switches from opencensus to promhttp for metrics exporting in pkg/instrumentation

@klihub klihub force-pushed the metrics/refactor branch 13 times, most recently from a7477a6 to 14d10a3 Compare November 11, 2024 10:46
@klihub klihub changed the title metrics: cleanup/refactor metrics registration, collection and gathering. [1/4] metrics: cleanup metrics registration, collection and gathering. Nov 11, 2024
@klihub klihub force-pushed the metrics/refactor branch 2 times, most recently from ff456df to 772f60f Compare November 11, 2024 22:55
@klihub klihub changed the title [1/4] metrics: cleanup metrics registration, collection and gathering. [1/5] metrics: cleanup metrics registration, collection and gathering. Nov 11, 2024
Copy link

@pfl pfl left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub force-pushed the metrics/refactor branch 4 times, most recently from fc7babb to 7434f00 Compare November 13, 2024 09:45
@klihub klihub marked this pull request as ready for review November 13, 2024 13:29
@klihub klihub changed the title [1/5] metrics: cleanup metrics registration, collection and gathering. [stack: 1/5] metrics: cleanup metrics registration, collection and gathering. Nov 13, 2024
@klihub klihub changed the title [stack: 1/5] metrics: cleanup metrics registration, collection and gathering. [stacked 1/5] metrics: cleanup metrics registration, collection and gathering. Nov 13, 2024
@klihub klihub force-pushed the metrics/refactor branch 11 times, most recently from 0dcfab6 to d716f0f Compare November 18, 2024 12:53
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

Looks great!

@klihub, would you prefer to address my nits & supernits (to the extent you find reasonable) now or later? I'm fine merging this as is.

docs/resource-policy/developers-guide/architecture.md Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@klihub
Copy link
Collaborator Author

klihub commented Nov 19, 2024

Looks great!

@klihub, would you prefer to address my nits & supernits (to the extent you find reasonable) now or later? I'm fine merging this as is.

Let me take a look at them and see how many I could address right now.

Rework our metrics collector registry to take care of most
of the necessary bits for metrics registration, collection
and gathering. Use the prometheus-provided namespacing and
subsystems to put all generated metrics under a prefix and
provide additional grouping.

Signed-off-by: Krisztian Litkey <[email protected]>
Update cgroupstats collector for the reworked metrics registry.
Split out automatic registration to a register subpackage.

Signed-off-by: Krisztian Litkey <[email protected]>
Remove the old resmgr-triggered polling of policy metrics
and the old resmgr-level polling policy metrics collector.
Implement policy metrics collection in the policy package
itself.

Signed-off-by: Krisztian Litkey <[email protected]>
Remove the old opencensus-based prometheus exporter. Rework
prometheus exporting using our update metrics registry and
a promhttp /metrics-handler.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub
Copy link
Collaborator Author

klihub commented Nov 19, 2024

Looks great!
@klihub, would you prefer to address my nits & supernits (to the extent you find reasonable) now or later? I'm fine merging this as is.

Let me take a look at them and see how many I could address right now.

@askervin I tried to address all your comments.

Add configuration bits for controlling which metrics are
collected. Enable collection of policy metrics by default.

Signed-off-by: Krisztian Litkey <[email protected]>
Remove obsolete and unused option entries. Give a warning about
using the now-obsolete '-metrics-interval' argument. It's used
unconditionally by our existing Helm charts, so we'll phase it
out a bit more gently.

Signed-off-by: Krisztian Litkey <[email protected]>
@askervin askervin merged commit 4a22287 into containers:main Nov 20, 2024
4 checks passed
@klihub klihub deleted the metrics/refactor branch November 20, 2024 08:14
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.

4 participants