-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
promlint: Add timestamp_seconds suffix restriction #1520
Conversation
fddb26c
to
5810fbe
Compare
Allow using timestamp_seconds suffix only for gauge metrics. Signed-off-by: Assaf Admi <aadmi@redhat.com>
5810fbe
to
e7c96fd
Compare
@ArthurSens Can you please review this PR? |
Isn't there a linter that already fails if a counter doesn't end with |
@ArthurSens For counters yes, but it won't catch other metrics types using the |
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.
Thanks! Trying to understand the exact problems we want to guard against, but the current PR. is not ideal at the moment I think
name: "counter with _timestamp_seconds suffix", | ||
in: ` | ||
# HELP _timestamp_seconds Test metric. | ||
# TYPE _timestamp_seconds counter |
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.
Side note: Metrics starting with underscore should be a problem too 🙈
@@ -499,6 +499,63 @@ x_bytes 10 | |||
runTests(t, tests) | |||
} | |||
|
|||
func TestLintGauge(t *testing.T) { |
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.
LintGauge tests counters?
Text: `counter metrics should have "_total" suffix`, | ||
}, | ||
{ | ||
Metric: "_timestamp_seconds", |
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 does not feel helpful for counters, the above problem duplicates here kind of and is kind of awkward why timestamp_second
is not allowed. Can I do ts_seconds
?
|
||
// LintGauge detects issues specific to gauges, as well as patterns that should | ||
// only be used with gauges. | ||
func LintGauge(mf *dto.MetricFamily) []error { |
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.
Hm, so this feels bit wrong. Name suggests we lint gauges, but looks like we simply lint non-gauges. I would move this to related types one by one.... but is there one?
For counters we don't need this, because _total check would be failed. For histograms and summaries, I think they are always cumulative so this check should be there too (I don't think it's implemented?).
So what we are left with? On what case you got hit by this?
@bwplotka thanks for your review :) |
What this PR does
Add a rule to promlint, restricting
timestamp_seconds
suffix for non-gauge metrics.timestamp_seconds
metrics are not cumulative and shouldn't be split to buckets or quantiles. They should be of gauge type.For example, in https://kubernetes.io/docs/reference/instrumentation/metrics/#list-of-stable-kubernetes-metrics and https://github.com/kubernetes/kube-state-metrics/tree/main/docs/metrics, all
timestamp_seconds
metrics are gauges.