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

Kanister Prometheus metrics skeleton framework #2163

Merged
merged 26 commits into from
Aug 10, 2023
Merged

Kanister Prometheus metrics skeleton framework #2163

merged 26 commits into from
Aug 10, 2023

Conversation

mellon-collie
Copy link
Contributor

Change Overview

Basic framework for instrumenting Kanister with Prometheus metrics and exporting them. This framework is based on the design document mentioned in this PR: #2151

This PR creates a new "metrics" package that exports methods to initialize and register different Prometheus metrics: CounterVec, GaugeVec, HistogramVec, Counter, Gauge and Histogram. It also exposes a BoundedLabel struct type that allows consumers to specify the bounded number of labels and label values to prevent label cardinality bombs.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  1. The following unit tests have been added:

a) TestGetLabelCombinations: This method tests the backtracking algorithm that generates the label combinations that are to be initialized for either a CounterVec, a GaugeVec or a HistogramVec.

b) TestInitCounterVec: This method tests if whether the initialized the Counters within a newly created CounterVec has been set to 0.

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@github-actions
Copy link
Contributor

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

@mellon-collie mellon-collie force-pushed the kanister-prometheus-skeleton-framework branch from da871d3 to 6dd98fb Compare July 12, 2023 15:10
viveksinghggits
viveksinghggits reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

I tried to look into the PR but don't have much valuable inputs. Someone else needs to review the PR.

pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@mellon-collie mellon-collie force-pushed the kanister-prometheus-skeleton-framework branch from 513c0bb to a45e0b1 Compare July 31, 2023 17:20
mellon-collie and others added 10 commits July 31, 2023 13:21
@mellon-collie
Added new metrics package
4d74cf7
@mellon-collie
Added backtracking logic to generate label combinations
6c68996
@mellon-collie
Fixed issue with empty combinations
4c42785
@mellon-collie
Added label combinations unit test
7b2a9e4
@mellon-collie
Added unit test for init counter vec
65e8e44
@mellon-collie
added comments
11948c6
@mellon-collie
Fixed comments
2642098
@mellon-collie
Addressed Mark's review comments:
413f4ea 
1. Create a consistent interface for all methods - instead of passing just labels []string, pass BoundedLabels
2. Based on Point 1, to represent unbounded label values, add sentinel markers
  a) For countervecs, panic if sentinel values are passed
3. In TestInitCounterVec, check if a default registerer has no registered counters initially.
4. Switch the assert checks in TestInitCounterVec
@mellon-collie
Fixed comments
e0968ff
@mellon-collie
Addressed Vivek's review comments
fba4fb4
@mellon-collie mellon-collie force-pushed the kanister-prometheus-skeleton-framework branch from a45e0b1 to fba4fb4 Compare July 31, 2023 17:21
@ewhamilton ewhamilton mentioned this pull request Aug 2, 2023
Merged
7 tasks
ewhamilton
ewhamilton reviewed Aug 3, 2023
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
ewhamilton
ewhamilton reviewed Aug 3, 2023
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
ewhamilton
ewhamilton reviewed Aug 3, 2023
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@ewhamilton
Copy link
Contributor

@mellon-collie Good job. If we needed to ship this today, I would approve it. But I'd like to discuss with you (via this ticket for future reference) my comments.

@mellon-collie mellon-collie force-pushed the kanister-prometheus-skeleton-framework branch from a4743c1 to 0257445 Compare August 3, 2023 05:13
ewhamilton
ewhamilton reviewed Aug 4, 2023
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pavannd1
pavannd1 reviewed Aug 9, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics_test.go Show resolved Hide resolved
pkg/metrics/metrics_test.go Outdated Show resolved Hide resolved
pkg/metrics/metrics_test.go Outdated Show resolved Hide resolved
pkg/metrics/metrics_test.go Outdated Show resolved Hide resolved
pkg/metrics/metrics_test.go Outdated Show resolved Hide resolved
pavannd1
pavannd1 reviewed Aug 9, 2023
View reviewed changes
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

Looks good overall. Minor suggestions.

@mellon-collie
Copy link
Contributor Author

Looks good overall. Minor suggestions.

Thanks for the review. I've addressed all the review comments.

miquella
miquella approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@miquella miquella 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! Just a few nits 😃

pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@mellon-collie mellon-collie force-pushed the kanister-prometheus-skeleton-framework branch from a98b800 to a02bae6 Compare August 9, 2023 20:12
pavannd1
pavannd1 approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

Missing license on the test file. Looks great otherwise.

The CI failure might be due to a change @r4rajat introduced. The fix is in progress.

pkg/metrics/metrics_test.go Show resolved Hide resolved
@mellon-collie
Copy link
Contributor Author

mellon-collie commented Aug 10, 2023 •
edited
Loading

Missing license on the test file. Looks great otherwise.

The CI failure might be due to a change @r4rajat introduced. The fix is in progress.

Thanks for pointing that out @pavannd1 . It looks like across the project we're a little bit inconsistent with adding licenses on test files - so of them do have it, while some don't. I've added it for metrics_test.go

pavannd1
pavannd1 approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

@mellon-collie You're right!
We need to educate both contributors and reviewers about it. We could find a linter to enforce it too.

@mergify mergify bot merged commit cfa57f5 into master Aug 10, 2023
14 checks passed
@mergify mergify bot deleted the kanister-prometheus-skeleton-framework branch August 10, 2023 16:47
@pavannd1
Copy link
Contributor

@mellon-collie I'll be reverting this PR in the interest of unblocking the release. Let's re-merge post release.

pavannd1 added a commit that referenced this pull request Aug 11, 2023
@pavannd1
Revert "Kanister Prometheus metrics skeleton framework (#2163)"
3a8189d 
This reverts commit cfa57f5.
@pavannd1 pavannd1 mentioned this pull request Aug 11, 2023
Merged
mergify bot pushed a commit that referenced this pull request Aug 11, 2023
@pavannd1
Revert "Kanister Prometheus metrics skeleton framework (#2163)" (#2262)
dd714ba 
This reverts commit cfa57f5.
@mellon-collie
Copy link
Contributor Author

@mellon-collie I'll be reverting this PR in the interest of unblocking the release. Let's re-merge post release.

Sure, thanks @pavannd1

mellon-collie added a commit that referenced this pull request Aug 11, 2023
@mellon-collie
Revert "Revert "Kanister Prometheus metrics skeleton framework (#2163)…
c898d67 
…" (#2262)"

This reverts commit dd714ba.
mergify bot pushed a commit that referenced this pull request Aug 11, 2023
@mellon-collie
Revert "Revert "Kanister Prometheus metrics skeleton framework (#2163… (
c1e4639 
#2265)

* Revert "Revert "Kanister Prometheus metrics skeleton framework (#2163)" (#2262)"

This reverts commit dd714ba.

* Added _ import for metrics pkg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viveksinghggits viveksinghggits viveksinghggits left review comments

@miquella miquella miquella approved these changes

@pavannd1 pavannd1 pavannd1 approved these changes

@ewhamilton ewhamilton ewhamilton approved these changes

Assignees
No one assigned
Labels
kueue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mellon-collie @ewhamilton @pavannd1 @miquella @viveksinghggits