Skip to content

Comments

Anchor silence regex rules#748

Merged
fabxc merged 1 commit intoprometheus:masterfrom
prymitive:anchor-rules
May 9, 2017
Merged

Anchor silence regex rules#748
fabxc merged 1 commit intoprometheus:masterfrom
prymitive:anchor-rules

Conversation

@prymitive
Copy link
Contributor

@prymitive prymitive commented Apr 27, 2017

When user creates a new silence with regex match it's left to the user to anchor it, which is not consistent with Prometheus regex handling (promql/functions.go#L818).

CC @stuartnelson3

When user creates a new silence with regex match it's left to the user to anchor it, which is not consistent with Prometheus regex handling (promql/functions.go#L818).
@stuartnelson3
Copy link
Contributor

@fabxc Was there a reason for this not being anchored?

@fabxc
Copy link
Contributor

fabxc commented Apr 27, 2017

I think it should be consistent.
Possible that I just didn't consider building the initial version or we made the switch to anchoring in Prometheus afterwards.

There's an obvious problem here of course. Already created silences will stop working.
So either we just accept that and break people, or we make this change to anchor on silence creation time rather than match time. That will also spawn potentially confusing results of course when old un-anchored silences have a different behavior.

@brian-brazil

@brian-brazil
Copy link
Contributor

I think consistency is important. In addition the same arguments about overmatching due to no anchoring as apply in PromQL&relabelling also apply here.

How this is handled UI wise is a separate question.

@fabxc
Copy link
Contributor

fabxc commented Apr 27, 2017

@brian-brazil what I mean is, we should of course change it. But this could mean that users upgrading have existing silences that will no longer match.

Probably okay when coming with a disclaimer.

@brian-brazil
Copy link
Contributor

Yea, we'd want to signal this clearly.

@prymitive prymitive mentioned this pull request Apr 27, 2017
@prymitive
Copy link
Contributor Author

There's an obvious problem here of course. Already created silences will stop working.

I think this is more complicated - this change will either break or fix silence rules for people - depending on the intent they had when creating those, and that can easily vary from user to user in the same organisation using Prometheus.
I don't think current behavior is explicitly stated anywhere (I couldn't find anything) so it's hard to say what people assume right now, some docs on prometheus.io stating clearly what should people expect would be helpful.

@fabxc
Copy link
Contributor

fabxc commented Apr 28, 2017

I think this is more complicated

Absolutely. It should have been "will potentially stop working as before".

@fabxc
Copy link
Contributor

fabxc commented May 9, 2017

I will add this to 0.6.2 – potentially disruptive but as behavior was previously unspecified, it's not really a breaking change.

@fabxc fabxc merged commit c9163bd into prometheus:master May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants