-
Notifications
You must be signed in to change notification settings - Fork 42
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
metrics name linter: Remove timestamp_seconds suffix restriction #244
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avlitman why was this rule added? Maybe we have some reference that suggested the need for this rule, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was asked by Shirly in the initial ticket, in kubevirt the
timestamp_seconds
suffix is only for counters: https://github.com/kubevirt/kubevirt/blob/021fbac594c7d093d8ab8fb318ecd1667028e0d7/docs/metrics.md?plain=1#L96, but if it's in Kubernetes metrics maybe we should remove it, or change to both gauge and counter.@sradco WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @avlitman @assafad , I asked this because the prom-linter failed for the
timestamp_seconds
when we used counter, if my memory is correct.I think we can update the
timestamp_seconds
to gauge since they represent a point in time. I agree it more accurate this way.I agree with @assafad.
But in that case we would also need to update the rest of the relevant metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a bug for the other metrics.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but remove this test or change it to gauge? if we remove this test all types can have this suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to add gauge as an option in this test, then once metric names change to delete the counter.
But I don't have strong opinion about it, just make more sense to me.
and the custom_linter_rules.go is for custom rules we want to add, it can be anything basically, but I agree with trying to ask them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, you only restrict histogram/summary metrics to not use this suffix. Do we need it?
I don't see why it's specific to our needs (like the
kubevirt_suboperator
prefix is). If Prometheus maintainers tell us that all types should use this suffix, why do we need to enforce something? If they tell us that it's a good restriction, we can add it in promlint. Do we have any special usecase here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure so let's ask them and after they change it or say it's not needed let's remove here- because if it is needed we don't want new metric to be added with wrong suffix in the meanwhile- that's my concern.
If we decide it's only for gauge- deleting this test will make it possible to add new counters with this suffix.. and we don't want more names to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think adding histogram and summary metrics with this suffix, when it's not justified, is a use case, please say. If not, I don't think we need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sradco mention that we need to change counters with this suffix. no?