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

Detect/forbid (verbatim) duplicated lines in the rules files #912

Open
beorn7 opened this Issue Jul 21, 2015 · 23 comments

Comments

Projects
None yet
8 participants
@beorn7
Copy link
Member

beorn7 commented Jul 21, 2015

Accidentally, I found a case where the same rule name was used twice. That went without complaints by the server, and if both rule evaluations had resulted in different label sets, it might have just worked. However, they resulted in the same label set, so the results were funneled into the same time series, and now with our fancy monotonicity detector, we actually see warnings.

Question: Should we in general forbid to use the same rule name twice? Or are their use cases where you want to use the same rule name (but create different label values for the result)?

I tend to simply ban duplicate rule names, but perhaps I'm missing something.

@juliusv @brian-brazil

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 21, 2015

I'd say the current behaviour is correct.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 21, 2015

Found more duplicate rule files at SC... happens quite often, and goes completely undetected so far.

@brian-brazil What's the use case for the current behavior?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 21, 2015

As you said in the OP, when you have different label values for each.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 21, 2015

Let me rephrase: What would be a use case for creating two rules with the same name that have different label values?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 21, 2015

If different teams had per-job rules, those applying to the Standard Exports would be in this situation.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 21, 2015

I guess that's a valid point... we have one server per team at SC, so basically one team manages all the rules on one server.

But we could detect if a rule is completely identical to another one. Would that make sense?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 21, 2015

We could, but it'd break the abstraction that each rule runs every eval interval. It'll also cause oddities when we have ordered rule evaluation.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 21, 2015

I mean, we could simply reject to load the rules file if we have an exact duplicate.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 21, 2015

I'm not sure there's much value to that - should we also bail out if there's two rules that are effectively equal too?

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 21, 2015

It might be difficult to detect “effectively equal” rules.

But on at least three servers at SC, I found rule pairs that funneled into the same time series. One of them was basically a typo, i.e. two different expressions put their results into the same time series (bad). The other two were exact duplicates (copy&paste problem). The related time series had therefore duplicated samples, and with 0.15.0, those samples got rejected, and a counter got incremented on which you probably want to alert. So catching exactly duplicated rules will help to prevent a mistake that will later page you...

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented Jul 21, 2015

I did use this feature (no quotes) to feed different queries / metrics into one uber-metric with different labels (@beorn7 this is on a branch r/n, I can show you). I think we should keep allowing them, and we can't tell beforehand that the labelsets will be different.

I would vote to make a note of this in the documentation but allow people shooting themselves in the foot.

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented Jul 21, 2015

… and if people get paged for this then that's exactly the right point in time to take note of it, because it's the only point when we can be sure it is a problem.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jul 21, 2015

Agreed, this sounds more like an example of bad external rule / config management than one with Prometheus per se. Would also vote to keep the current behavior.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 22, 2015

Still, if i have two or more literally (in the literal sense of "literally") identical rules, there is no way that can be meaningful. So should we at least error in that case?

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jul 22, 2015

I'd be ok with erroring in that case. I'd say the usefulness is limited (slight decrease in computing resources, but the storage will just reject the second copy of identical samples anyways now), but it couldn't hurt.

@beorn7 beorn7 changed the title Detect/forbid duplicate rule names? Detect/forbid (verbatim) duplicated lines in the rules files Jul 22, 2015

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 22, 2015

I have adjusted the subject.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented Jul 22, 2015

👍 to being strict about such issues

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 27, 2017

This is probably something for a warning in promtool, once we add a general rule linter

@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Dec 19, 2017

How does this play with rule-groups?

Duplicated rule in rule-group would be an error as all rules in the rule-group are evaluated with the same timestamp. Also, duplicated rule across groups can also cause out-of-order appends.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Dec 19, 2017

Duplicated rule in rule-group would be an error as all rules in the rule-group are evaluated with the same timestamp. Also, duplicated rule across groups can also cause out-of-order appends.

Not necessarily, each rule could be one iteration of a state machine and you can construct those to avoid duplicate timestamps.

@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Jan 18, 2018

But how would avoid the entire rule-group being evaluated at the same timestamp? The same rule in different groups might make sense, but the same rule in the same group will be either a nop or an error right?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jan 18, 2018

You're thinking of normal simple rules, self-referential rules could have different effects when repeated.

@totallyunknown

This comment has been minimized.

Copy link

totallyunknown commented Mar 6, 2018

Can we add a duplicate rule name detection into promtool?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.