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

metric: provide way to declare labels. #27835

Merged
merged 3 commits into from Nov 21, 2023
Merged

metric: provide way to declare labels. #27835

merged 3 commits into from Nov 21, 2023

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Aug 30, 2023 •
edited 

metric: provide way to declare labels.

Adds *WithLabels Vec[T] constructors for histogram/gauge/counter.
This provides a mechanism to declare a metrics possible range of label values up front.
The xxxWithLabels functions will automatically initialize such metrics
such that they will be exported with zero values by the prometheus
endpoint.

One of the stated best practices for Prometheus instrumentation is
avoiding missing metrics [1].

This change will make it easier to provide metrics that are initialized
correctly to produce more reliable metrics.

[1] https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics

Pre-initialize several known metric vectors to avoid empty metrics (specifically: endpoint_regenerations_total, policy_change_total,  policy_implementation_delay, policy_l7_total and kubernetes_events metrics).

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 30, 2023
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/metric-labels-v2 branch 3 times, most recently from 56a82b0 to dea86e1 Compare September 12, 2023 19:56
@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 13, 2023
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/metric-labels-v2 branch 4 times, most recently from 62b3457 to 2a2c318 Compare October 27, 2023 05:43
@tommyp1ckles tommyp1ckles added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Oct 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 27, 2023
@tommyp1ckles tommyp1ckles marked this pull request as ready for review October 27, 2023 05:44
@tommyp1ckles tommyp1ckles requested review from a team as code owners October 27, 2023 05:44
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/metric-labels-v2 branch 3 times, most recently from d3f704b to 7e91bf4 Compare October 27, 2023 05:56
@maintainer-s-little-helper
Copy link

Commit c3672f9 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 27, 2023
@maintainer-s-little-helper
Copy link

Commit c3672f9 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 27, 2023
@tommyp1ckles
Copy link
Contributor Author

tommyp1ckles commented Nov 13, 2023 •
edited

@derailed @squeed addressed/responded to review points, as well I moved the collections package into being a part of pkg/metrics/metric 😄 PTAL

derailed
derailed requested changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@tommyp1ckles Tx for the updates! a few more picks...

pkg/metrics/metric/collections/product_test.go Outdated Show resolved Hide resolved
pkg/metrics/metric/collections/product_test.go Show resolved Hide resolved
pkg/metrics/metric/collections/product.go Outdated Show resolved Hide resolved
pkg/metrics/metric/collections/product.go Outdated Show resolved Hide resolved
pkg/metrics/metric/counter.go Show resolved Hide resolved
pkg/metrics/metric/metric.go Show resolved Hide resolved
pkg/metrics/metric/metric.go Show resolved Hide resolved
pkg/metrics/metric/metric.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/metric-labels-v2 branch 3 times, most recently from 7401193 to 6bf83f2 Compare November 14, 2023 20:12
@tommyp1ckles
Copy link
Contributor Author

/test

derailed
derailed requested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@tommyp1ckles LG!

pkg/metrics/metric/gauge.go Outdated Show resolved Hide resolved
pkg/metrics/metric/counter.go Outdated Show resolved Hide resolved
pkg/metrics/metric/gauge.go Outdated Show resolved Hide resolved
pkg/metrics/metric/histogram.go Outdated Show resolved Hide resolved
pkg/metrics/metric/metric.go Outdated Show resolved Hide resolved
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/metric-labels-v2 branch 3 times, most recently from 3a69ef8 to 1167559 Compare November 16, 2023 04:31
@tommyp1ckles
Copy link
Contributor Author

/test

derailed
derailed approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@tommyp1ckles Tx for the updates!

pkg/metrics/metric/gauge.go Show resolved Hide resolved
@tommyp1ckles
Copy link
Contributor Author

/ci-clustermesh

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/metric-labels-v2 branch 2 times, most recently from 1d7a01b to 7b98652 Compare November 17, 2023 04:58
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles
metric/collections: generic function to compute cartesian product.
7b243be 
This will be used in subsequent commits to implement metric label
initialization.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles
metric: provide way to declare labels.
62bbe0f 
Adds *WithLabels Vec[T] constructors for histogram/gauge/counter.
This provides a mechanism to declare a metrics possible range of label values up front.
The xxxWithLabels functions will automatically initialize such metrics
such that they will be exported with zero values by the prometheus
endpoint.

One of the stated best practices for Prometheus instrumentation is
avoiding missing metrics [1].

This change will make it easier to provide metrics that are initialized
correctly to produce more reliable metrics.

[1] https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles
metrics: initialize metrics for several existing metrics.
bd5ec0b 
Using previous commits, initializes metrics to zero for various existing metrics.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 17, 2023
@lmb lmb merged commit 245f634 into main Nov 21, 2023
211 of 213 checks passed
@lmb lmb deleted the pr/tp/metric-labels-v2 branch November 21, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@derailed derailed derailed approved these changes

@squeed squeed squeed approved these changes

@chancez chancez chancez approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

Assignees
No one assigned
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tommyp1ckles @derailed @squeed @chancez @dylandreimerink @bimmlerd @lmb