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

Extend metrics library #2606

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Extend metrics library #2606

merged 2 commits into from
Jul 22, 2024

Conversation

lambdanis
Copy link
Contributor

@lambdanis lambdanis commented Jun 24, 2024

This is a general rewrite of pkg/metrics library, but it a fully backwards
compatible way. Goals and key components are described in doc.go, so I would
suggest starting the review there :)

Here is the Tetragon metrics framework proposal, covering the bigger picture including future plans. The "Goals" and "Proposal/Library" sections are copied into doc.go.

For usage examples see #2604 (defining health metrics group, migrating some custom metrics & collectors to the new interface).

Fixes: #2376

@lambdanis lambdanis added area/metrics Related to prometheus metrics release-note/misc This PR makes changes that have no direct user impact. labels Jun 24, 2024
@lambdanis lambdanis requested a review from a team as a code owner June 24, 2024 19:21
This was referenced Jun 24, 2024
@kevsecurity kevsecurity removed their request for review June 27, 2024 09:33
@kkourt kkourt self-requested a review June 27, 2024 09:45
@willfindlay willfindlay self-requested a review June 28, 2024 15:23
@lambdanis
Copy link
Contributor Author

I pushed some commits with small fixes. I will squash the commits after the review, just didn't want to force push now.

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks for providing doc.go, it was very helpful in trying to understand the intent of this PR.
Overall, the ideas make sense to me but the implementation is not easy to follow.

pkg/metrics/opts.go Show resolved Hide resolved
pkg/metrics/filteredlabels.go Show resolved Hide resolved
pkg/metrics/group.go Outdated Show resolved Hide resolved
pkg/metrics/doc.go Outdated Show resolved Hide resolved
// - cardinality can be constrained
// - support for configurable labels
// - metric is initialized at startup for known label values
// - metric is automatically included in generated docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the case then that granular metrics are metrics with the above properties?

Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit e0284e8
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/669aded4197ff600080b26bc
😎 Deploy Preview https://deploy-preview-2606--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks Anna, LGTM.

If it's not too much hassle, please fold the last commit (metrics: Simplify Group interface) with the second commit (Extend metrics library).

@lambdanis
Copy link
Contributor Author

If it's not too much hassle, please fold the last commit (metrics: Simplify Group interface) with the second commit (Extend metrics library).

Sure, I think I'll squash all commits following "Extend metrics library" into it. I separated some code reshuffling into the first commit, so that it's clearer what code is new and what is just moved.

@lambdanis
Copy link
Contributor Author

I added two commits with fixes, will squash them too.

This commit is a preparation for the further extension of pkg/metrics. There
are no functional changes, only moving code around:
- moved helpers for creating metrics from metricwithpod.go and
  granularmetric.go to per-type files: counter.go, gauge.go and histogram.go
- renamed labels.go to filteredlabels.go

Signed-off-by: Anna Kapuscinska <anna@isovalent.com>
This is a general rewrite of pkg/metrics library, but it a fully backwards
compatible way. Goals and key components are described in doc.go.

Fixes: #2376

Signed-off-by: Anna Kapuscinska <anna@isovalent.com>
@lambdanis lambdanis merged commit 5e23182 into main Jul 22, 2024
45 checks passed
@lambdanis lambdanis deleted the pr/lambdanis/library branch July 22, 2024 18:56
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.

Improve metrics library to enforce best practices
2 participants