-
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
Conversation
Signed-off-by: assafad <aadmi@redhat.com>
// Check "_timestamp_seconds" suffix for non-counter metrics | ||
if *mf.Type != dto.MetricType_COUNTER && strings.HasSuffix(*mf.Name, "_timestamp_seconds") { | ||
problems = append(problems, promlint.Problem{ | ||
Metric: *mf.Name, | ||
Text: "non-counter metric should not have \"_timestamp_seconds\" 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.
@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.
I prefer to add gauge as an option in this test, then once metric names change to delete the counter.
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?
WDYM? I linked a list of metrics, and mentioned all the |
I deleted my comment it was by mistake I missed it. |
/approve |
…ds-suffix metrics name linter: Remove timestamp_seconds suffix restriction
What this PR does / why we need it:
This PR removes the linter's
timestamp_seconds
suffix restriction for non-counter metrics, as gauge metrics should be allowed to use it. Timestamp metrics are usually not cumulative, and should be of gauge type.For example, in https://kubernetes.io/docs/reference/instrumentation/metrics/#list-of-stable-kubernetes-metrics all the
timestamp_seconds
metrics are gauges.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: