Icinga / icinga2

The core of our monitoring platform with a powerful configuration language and REST API.
https://icinga.com/docs/icinga2/latest
GNU General Public License v2.0
1.99k stars 573 forks source link

Semantics of severity value changed #9130

Open tktr opened 2 years ago

tktr commented 2 years ago

Describe the bug

Since release 2.12.0, the semantics of the severity value changed for both host and service. However, the documentation still refers to the old semantics as it was introduced with 2.11. Furthermore, the release notes do not mention this backwards incompatible change.

To Reproduce

  1. Trigger Notification for critical alert with Icinga 2.11.x and pass $severity$ variable
  2. Upgrade to > 2.12
  3. Retrigger same Notification

Expected behavior

The severity value should not change or the change should be documented.

Your Environment

Additional context

The severity value is actually quite handy when used i.e. together with Notifications as it provides all the state information needed in a single argument. However, if the value is not stable it can't be used for such a use case. The comment seems to imply that the sole purpose here is to make states sortable for Icingaweb2. So should the value be used at all outside the context of sorting?

julianbrost commented 2 years ago

The comment seems to imply that the sole purpose here is to make states sortable for Icingaweb2. So should the value be used at all outside the context of sorting?

Indeed, I've just done some digging around into the history of the severity attribute and it was intended to use as sort order from the very beginning (#5117). How it ended up in the current state is a bit unfortunate, you can have a look at the history section of https://github.com/Icinga/icinga2/pull/9196#pullrequestreview-864847849 for more details.

For this reason, especially that documenting the internal structure of the severity value make it hard to impossible to tweak the sort order in the future, the current plan is to document it as an opaque value that should only be used for ordering.

The severity value is actually quite handy when used i.e. together with Notifications as it provides all the state information needed in a single argument. However, if the value is not stable it can't be used for such a use case.

For this, a new bit set could be added with a clear description that it's a combination of some bit flags with the only change ever happening is more flags being added. But I think fiddling with such bit sets is always a bit annoying as you somewhat have to copy around magic numbers between projects. So another idea might be to allow easily serializing either the full object or maybe just its current state as JSON and pass this instead. What do you think of something like this?

Al2Klimov commented 2 years ago

What's the problem with having to pass -x $X, -y $Y and -z $Z explicitly to $COMMAND?

julianbrost commented 1 year ago

The bug part of this issue was addressed by #9198 which basically manifested the following:

The comment seems to imply that the sole purpose here is to make states sortable for Icingaweb2. So should the value be used at all outside the context of sorting?

Now the question is if there's anything left to do? I think this boils down to if there's a good example where some combined value would be helpful. At the moment, I fail to see it. Just passing the required attributes as part of the command object makes that definition a bit longer, but provides good flexibility and saves you from interpreting bit sets (not hard technically, but needs constants from icinga2).