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
2.03k stars 578 forks source link

Icinga DB: Ensure all timestamps are in milliseconds #8991

Closed julianbrost closed 3 years ago

julianbrost commented 3 years ago

In Icinga DB heartbeats (i.e. icinga:stats messages), the startup time is currently in seconds, whereas other absolute timestamps are in milliseconds. Internally, we discussed today that we want to fix this in icinga2, even if this means that we have to require some icinga2 > 2.13.0 for icingadb 1.0.

So change this timestamp to milliseconds and also verify that there are no other timestamps left that are in seconds.

Other issues identified:

refs https://github.com/Icinga/icingadb/issues/357

nilmerg commented 3 years ago

this means that we have to require some icinga2 > 2.13.0 for icingadb 1.0.

It's no hard requirement. So anyone still using 2.13 can do so, but just won't see the correct start time. :man_shrugging:

julianbrost commented 3 years ago

Part of the discussion was that because of https://github.com/Icinga/icinga2/pull/8953, we will probably end up with such a requirement anyways if we don't want to implement a workaround for that in icingadb, so we can fix this timestamp as well.

julianbrost commented 3 years ago

So I've looked over the code and everywhere a timestamp is explicitly written to Redis, it is done in milliseconds. The start time is special as this is just taken from IcingaApplication::StatsFunc(): https://github.com/Icinga/icinga2/blob/95cdc00ad4e1ab788794322c92c54cd3f5c8c2bf/lib/icinga/icingaapplication.cpp#L84

Then there are also intervals/durations: almost all of these are given in seconds, with one exception: check_timeout in states (which it looks like Icinga DB doesn't even sync to MySQL, does Icinga DB Web use those values?): https://github.com/Icinga/icinga2/blob/95cdc00ad4e1ab788794322c92c54cd3f5c8c2bf/lib/icingadb/icingadb-objects.cpp#L2286-L2289

nilmerg commented 3 years ago

does Icinga DB Web use those values?

Yes. But only in the inspection detail:

From Redis as milliseconds: https://github.com/Icinga/icingadb-web/blob/master/library/Icingadb/Common/ObjectInspectionDetail.php#L133 From Db as seconds: https://github.com/Icinga/icingadb-web/blob/master/library/Icingadb/Common/ObjectInspectionDetail.php#L168

But indeed, only the Redis section shows a timeout.

Al2Klimov commented 3 years ago

@julianbrost Huh? One refs PR, but no fixes one?

julianbrost commented 3 years ago

8998 fixes one timestamp, but there might be others that could still be wrong (I haven't seen any others so far though).

julianbrost commented 3 years ago

flexible_duration is inconsistent between the downtime and downtime_history tables:

https://github.com/Icinga/icinga2/blob/6b3a2ee42993f34226ef53db0d5773746c924bc5/lib/icingadb/icingadb-objects.cpp#L1333 https://github.com/Icinga/icinga2/blob/6b3a2ee42993f34226ef53db0d5773746c924bc5/lib/icingadb/icingadb-objects.cpp#L1700 https://github.com/Icinga/icinga2/blob/6b3a2ee42993f34226ef53db0d5773746c924bc5/lib/icingadb/icingadb-objects.cpp#L1786

julianbrost commented 3 years ago

Two things brought up in this issue have been fixed:

There's one thing left, but that one is confusing me: https://github.com/Icinga/icinga2/issues/8991#issuecomment-922667881 Icinga DB itself (i.e. the daemon written in Go) should not convert between seconds and milliseconds anywhere, so it sounds wrong to me that web expects to read the same value as milliseconds from Redis but as seconds from SQL. Maybe @nilmerg can shed some light on this?

nilmerg commented 3 years ago

that one is confusing me

Because @nilmerg incorrectly interpreted the code and thought it's read from the DB while in fact it's read from the Icinga 2 API. :grin:

julianbrost commented 3 years ago

Ok, then there should be nothing more to do here.