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

Define health metrics group #2714

Merged
merged 4 commits into from
Jul 25, 2024
Merged

Conversation

lambdanis
Copy link
Contributor

There are two things happening here:

  1. Refactored custom collectors to use new helpers from pkg/metrics (commits 1-3). The main win here is simplifying how custom metrics are included in reference docs - we don't need to define a separate type and register it separately just for generating docs.
  2. Defined a health metrics group (last commit). The idea is that in the future this group will have constrained cardinality and will be enabled by default (in contrast to another group with potentially high cardinality "debug" metrics). This commit only refactors the existing metrics initialization code to use the new framework. The health metrics group contains all metrics that were documented in the "health metrics" section, but in the future some of them will likely be moved to another group.

I had this PR already: #2604 stacked on top of #2606, but then I merged the latter, the base branch got deleted, the PR got closed and I can't reopen it, so opening it again.

Use new helpers from pkg/metrics to define custom metrics and collector.

This required defining msg_op label. Before, this label was semi-constrained -
the opcodes are known, but in case we hit an unknown opcode (due to a bug), we
would still put it in the label. Now this label is defined as constrained, but
the collector still doesn't enforce the constraint in any way. The idea is that
in the future this label will be fully constrained and shared between different
metrics, (currently it's inconsistent - we use msg_op, op, opcode labels for the
same thing), so the label is defined in pkg/metrics.

Signed-off-by: Anna Kapuscinska <[email protected]>
Use new helpers from pkg/metrics to define custom metrics and collector.

Signed-off-by: Anna Kapuscinska <[email protected]>
Use new helpers from pkg/metrics to define custom metrics and collector.

Signed-off-by: Anna Kapuscinska <[email protected]>
The idea is that in the future this group will have constrained cardinality and
will be enabled by default (in contrast to another group with potentially high
cardinality "debug" metrics). This commit only refactors the existing metrics
initialization code to use the new framework. The health metrics group contains
all metrics that were documented in the "health metrics" section, but in the
future some of them will likely be moved to another group.

Signed-off-by: Anna Kapuscinska <[email protected]>
@lambdanis lambdanis added area/metrics Related to prometheus metrics release-note/misc This PR makes changes that have no direct user impact. labels Jul 22, 2024
@lambdanis lambdanis requested a review from a team as a code owner July 22, 2024 23:33
@lambdanis lambdanis requested a review from kevsecurity July 22, 2024 23:33
Copy link
Contributor

@kevsecurity kevsecurity left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe worth getting a review from someone (Chance?) who knows more about this area than me though.

@lambdanis lambdanis merged commit 42f268c into cilium:main Jul 25, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Related to prometheus metrics release-note/misc This PR makes changes that have no direct user impact.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants