utils.FromUnixMilli has floating point rounding issues

[oxzi]

Describe the bug

The utils.FromUnixMilli helper function internally converts the integer to a float and back again.

icingadb/pkg/utils/utils.go

Line 24 in 9c2dcd2

sec, dec := math.Modf(float64(ms) / 1e3)

By doing so, it adds additional precision which might result in a wrongly rounded number.

To Reproduce

While working on something totally different within icinga-notifications when I realized that my times.UnixMilli-based comparison simply didn't made any sense. After lots of debugging and enabling PostgreSQL's debug logs, I found an off-by-one issue with my timestamps and was able to reproduce it by adding another test case to TestUnixMilli_MarshalJSON.

diff --git a/pkg/types/unix_milli_test.go b/pkg/types/unix_milli_test.go
index 985fa2a..818cff0 100644
--- a/pkg/types/unix_milli_test.go
+++ b/pkg/types/unix_milli_test.go
@@ -1,6 +1,7 @@
 package types

 import (
+       "github.com/icinga/icingadb/pkg/utils"
        "github.com/stretchr/testify/require"
        "testing"
        "time"
@@ -16,6 +17,7 @@ func TestUnixMilli_MarshalJSON(t *testing.T) {
                {"zero", UnixMilli{}, `null`},
                {"epoch", UnixMilli(time.Unix(0, 0)), `0`},
                {"nonzero", UnixMilli(time.Unix(1234567890, 62500000)), `1234567890062`},
+               {"ilovefloats", UnixMilli(utils.FromUnixMilli(1714489037339)), `1714489037339`},
        }

        for _, st := range subtests {

Running this test results in an error, returning a number off by one.

=== RUN   TestUnixMilli_MarshalJSON/ilovefloats
    unix_milli_test.go:29: 
        	Error Trace:	…/icingadb/pkg/types/unix_milli_test.go:29
        	Error:      	Not equal: 
        	            	expected: "1714489037339"
        	            	actual  : "1714489037338"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-1714489037339
        	            	+1714489037338
        	Test:       	TestUnixMilli_MarshalJSON/ilovefloats
--- FAIL: TestUnixMilli_MarshalJSON (0.00s)
    --- FAIL: TestUnixMilli_MarshalJSON/ilovefloats (0.00s)

Expected :1714489037339
Actual   :1714489037338

Or, making it even more obvious by creating a test for utils.FromUnixMilli:

diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go
index b0ea54b..de34c76 100644
--- a/pkg/utils/utils_test.go
+++ b/pkg/utils/utils_test.go
@@ -3,6 +3,7 @@ package utils
 import (
        "github.com/stretchr/testify/require"
        "testing"
+       "time"
 )

 func TestChanFromSlice(t *testing.T) {
@@ -52,3 +53,8 @@ func requireClosedEmpty(t *testing.T, ch <-chan int) {
                require.Fail(t, "receiving should not block")
        }
 }
+
+func TestFromUnixMilli_Float(t *testing.T) {
+       const ms = 1714489037339
+       require.Equal(t, time.UnixMilli(ms), FromUnixMilli(ms))
+}

=== RUN   TestFromUnixMilli_Float
    utils_test.go:59: 
        	Error Trace:	…/icingadb/pkg/utils/utils_test.go:59
        	Error:      	Not equal: 
        	            	expected: time.Date(2024, time.April, 30, 16, 57, 17, 339000000, time.Local)
        	            	actual  : time.Date(2024, time.April, 30, 16, 57, 17, 338999986, time.Local)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,2 @@
        	            	-(time.Time) 2024-04-30 16:57:17.339 +0200 CEST
        	            	+(time.Time) 2024-04-30 16:57:17.338999986 +0200 CEST
        	            	 
        	Test:       	TestFromUnixMilli_Float
--- FAIL: TestFromUnixMilli_Float (0.00s)

Expected :time.Date(2024, time.April, 30, 16, 57, 17, 339000000, time.Local)
Actual   :time.Date(2024, time.April, 30, 16, 57, 17, 338999986, time.Local)

Expected behavior

No additional floating point precision should be added and later cut off while being cast back to an integer.

Your Environment

Include as many relevant details about the environment you experienced the problem in

• Icinga DB version: current main
• Icinga 2 version: N/A

Additional context

#744 and #747 might solve this by switching to Go's native time.UnixMilli.

[oxzi]

The function was removed in 3569b35.