Time releated fixes #466

bshifter · 2025-01-23T18:32:37Z

A bunch of datetime releated fixes:

dump functions for wallclock time and unixtime (they were extermely useful, so i kept them)
fix: filterx datetime string formatting error: unnecessary dash removed
prepartion for nano second precision support: timeutils/misc extended, typo fixed
fix: unix_time_to_unix_epoch calculation
fix: otel nanosecond-precision fields setter/getter
unit test for otel nanosec precision and timezone information

bazsi · 2025-01-23T19:51:06Z

lib/filterx/object-datetime.c

@@ -69,8 +69,6 @@ _convert_unix_time_to_string(const UnixTime *ut, GString *result, gboolean inclu
    {
      if (ut->ut_gmtoff >= 0)
        g_string_append_c(result, '+');
-      else
-        g_string_append_c(result, '-');


good catch. a unit test for this would be great somewhere.

bazsi · 2025-01-23T19:52:24Z

lib/timeutils/unixtime.h

@@ -69,6 +69,37 @@ unix_time_is_timezone_set(const UnixTime *self)
  return self->ut_gmtoff != -1;
 }

+static inline
+void dump_unix_time(const UnixTime *ut, GString *output)


can you move the return value type to the previous line? that's the convention we are using, e.g.

static inline void dump_unix_time(...)

also, we might hide this in the .c file, I don't see why this should be inline.

bazsi · 2025-01-23T19:54:14Z

lib/timeutils/wallclocktime.h

+#else
+  g_string_append_printf(output, "  GMT Offset: %ld\n", wct->wct_gmtoff);
+  g_string_append_printf(output, "  Timezone: %s\n", wct->wct_zone ? wct->wct_zone : "(null)");
+#endif


I don't see the difference in these two cases. The idea was to always have wct_gmtoff and wct_zone in WallClockTime so that we have those on all platforms. I don't think we need the conditional compilation.

bazsi · 2025-01-23T19:54:27Z

lib/timeutils/wallclocktime.h

@@ -124,6 +124,33 @@ struct _WallClockTime

 #endif

+static inline
+void dump_wall_clock_time(const WallClockTime *wct, GString *output)


again, this can go to the implementation file and please move "void" to the previous line.

bazsi · 2025-01-23T19:58:57Z

lib/timeutils/unixtime.c

@@ -371,7 +371,7 @@ guint64
 unix_time_to_unix_epoch(const UnixTime ut)


this function name is misleading for me. I know this patch does not introduce these, but for me "time since unix epoch" is in seconds and not in any particular resolution (msec, usecs or nsecs).

I would rename these to unix_time_from_unix_epoch_usec() and unix_time_to_unix_epoch_usec() to show the
granularity.

Later (if needed we can add the sec/msec/nsec variants as well).

bazsi · 2025-01-23T20:04:33Z

lib/timeutils/unixtime.c

@@ -371,7 +371,7 @@ guint64
 unix_time_to_unix_epoch(const UnixTime ut)
 {
  gint32 gmtoff = (ut.ut_gmtoff != - 1) ? ut.ut_gmtoff : 0;
-  return (guint64)((ut.ut_sec + gmtoff) * USEC_PER_SEC + ut.ut_usec);
+  return (guint64)((ut.ut_sec - gmtoff) * USEC_PER_SEC + ut.ut_usec);


unit tests for this would be dearly needed.

bazsi · 2025-01-23T20:05:23Z

modules/grpc/otel/filterx/otel-field.cpp

@@ -269,7 +272,7 @@ class OtelDatetimeConverter : public ProtobufField
  FilterXObject *FilterXObjectGetter(Message *message, ProtoReflectors reflectors)
  {
    uint64_t val = reflectors.reflection->GetUInt64(*message, reflectors.fieldDescriptor);
-    UnixTime utime = unix_time_from_unix_epoch(val);
+    UnixTime utime = unix_time_from_unix_epoch(NSEC_TO_USEC(val));


with this, I'd probably introduce the unix_time_from_unix_epoch_nsec() function, so we can push the NSEC_TO_USEC to be within the function body and not here.

bazsi · 2025-01-23T20:05:48Z

modules/grpc/otel/filterx/otel-field.cpp

@@ -279,7 +282,7 @@ class OtelDatetimeConverter : public ProtobufField
    if (filterx_object_extract_datetime(object, &utime))
      {
        uint64_t unix_epoch = unix_time_to_unix_epoch(utime);
-        reflectors.reflection->SetUInt64(message, reflectors.fieldDescriptor, unix_epoch);
+        reflectors.reflection->SetUInt64(message, reflectors.fieldDescriptor, unix_epoch * NSEC_PER_USEC);


likewise, unix_time_unix_epoch_nsec() could do the multiplication

Removed unnecessary `-` to prevent double dashes in the output. Padding negative numbers requires some revise. Signed-off-by: shifter <[email protected]>

Signed-off-by: shifter <[email protected]>

alltilla · 2025-01-24T09:57:14Z

lib/timeutils/unixtime.c

 {
  gint32 gmtoff = (ut.ut_gmtoff != - 1) ? ut.ut_gmtoff : 0;
-  return (guint64)((ut.ut_sec + gmtoff) * USEC_PER_SEC + ut.ut_usec);
+  return (guint64)((ut.ut_sec - gmtoff) * USEC_PER_SEC + ut.ut_usec);


I don't think we should subtract the GMT offset here. UnixTime's ut_sec and ut_usec fields are storing values that correspond to UTC already, the GMT offset is a hint to the original timezone. If you change the timezone in a UnixTime object, you don't need to modify ut_sec and ut_usec.

Let me demonstrate this with a config:

log { source { example-msg-generator(num(1)); }; filterx { dt = isodate("2025-01-01T04:00:00-03:00"); $logmsg = dt; $js = json({"dt": dt}); $repr = string(dt); $int = int(dt); $double = double(dt); log = otel_logrecord(); log.time_unix_nano = dt; $otel_dt_as_int = int(log.time_unix_nano); $otel_dt_as_double = double(log.time_unix_nano); log.attributes.int = dt; $otel_int = log.attributes.int; }; destination { stdout(template("$logmsg\n$js\n$repr\n$int\n$double\n$otel_dt_as_int\n$otel_dt_as_double\n$otel_int\n")); }; };

$ ./install/sbin/syslog-ng -Fe [2025-01-24T10:54:56.623207] Setting current version as config version; version='4.9' [2025-01-24T10:54:56.623207] syslog-ng starting up; version='4.9.0.155.g8aed412.dirty' 1735714800.000000-3:00 {"dt":"1735714800.000000"} 2025-01-01T04:00:00.000-03:00 1735714800000000 1735714800 1735725600000000 1735725600 1735725600000000

From https://greenwichmeantime.com/articles/clocks/iso/ we can see that 1735714800 is the correct value.

We can deduce 2 things from this output:

We are not using the same logic for otel and the rest of the filterx code.

The otel logic is erroneous.

We should fix both. Let's fix this function and use it everywhere:

datetime -> logmsg (marshal())

datetime -> json (map_to_json())

datetime -> string (repr())

datetime -> int (typecast_integer())

datetime -> double (typecast_double())

datetime -> otel int (u64Field, i64Field)

datetime -> otel_datetime (OtelDatetimeConverter)

Wildly agree. Sorry that I missed this.

Ohh gosh! My bad, i've missed the concept of Unixtime, and thought this was a bug whole time. Let me clean up my mess!

Signed-off-by: shifter <[email protected]>

bazsi reviewed Jan 23, 2025

View reviewed changes

bshifter force-pushed the time-fixes branch from a1dd334 to 3063b6b Compare January 24, 2025 09:51

bshifter added 2 commits January 24, 2025 10:51

filterx: fix datetime conversion to string

03055ef

Removed unnecessary `-` to prevent double dashes in the output. Padding negative numbers requires some revise. Signed-off-by: shifter <[email protected]>

timeutils: prepare nano sec precision handling

3dcb830

Signed-off-by: shifter <[email protected]>

bshifter force-pushed the time-fixes branch 2 times, most recently from 7896aca to 964fb28 Compare January 24, 2025 09:56

alltilla reviewed Jan 24, 2025

View reviewed changes

bshifter force-pushed the time-fixes branch from 964fb28 to 3f7875e Compare January 24, 2025 10:59

bshifter added 4 commits January 24, 2025 13:05

time: fix unix_time_to_unix_epoch calculation

62cfb13

Signed-off-by: shifter <[email protected]>

otel: fix timestamp calculation for nanosec precision otel fields

ef83f0b

Signed-off-by: shifter <[email protected]>

time: add UnixTime dumper fn for debugging purposes

3a63778

Signed-off-by: shifter <[email protected]>

time: Add WallClockTime dumper function for debugging purposes

357652f

Signed-off-by: shifter <[email protected]>

bshifter force-pushed the time-fixes branch from 3f7875e to 357652f Compare January 24, 2025 12:14

bshifter requested review from bazsi and alltilla January 24, 2025 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time releated fixes #466

Time releated fixes #466

bshifter commented Jan 23, 2025

bazsi Jan 23, 2025

bazsi Jan 23, 2025

bazsi Jan 23, 2025

bazsi Jan 23, 2025

bazsi Jan 23, 2025

bazsi Jan 23, 2025 •

edited

Loading

bazsi Jan 23, 2025

bazsi Jan 23, 2025

bazsi Jan 23, 2025

alltilla Jan 24, 2025 •

edited

Loading

bazsi Jan 24, 2025

bshifter Jan 24, 2025

		@@ -371,7 +371,7 @@ guint64
		unix_time_to_unix_epoch(const UnixTime ut)

Time releated fixes #466

Are you sure you want to change the base?

Time releated fixes #466

Conversation

bshifter commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazsi Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alltilla Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazsi Jan 23, 2025 •

edited

Loading

alltilla Jan 24, 2025 •

edited

Loading