search

appclacks / mirabelle

A stream processing engine for monitoring
Eclipse Public License 1.0
95 stars 5 forks source link

pagerduty: avoid local timezone #8

Closed ninaspitfire closed 3 years ago

ninaspitfire commented 3 years ago

Make the Pagerduty IO use UTC time, not local time for formatting date strings.

Without this patch, the behaviour of the IO (and the tests) depends on the local timezone. The tests can only pass when TZ=UTC-1.

ninaspitfire commented 3 years ago

Updated this patch to treat epoch time as seconds not milliseconds. Am I correct in assuming that the :time of a Mirabelle event is in epoch seconds, like a Riemann event?

Thanks.

ninaspitfire commented 3 years ago

OK. I found in the documentation that event time is, indeed, measured in seconds. (Superb documentation, by the way). <3

mcorbin commented 3 years ago

Hello,

Thank you for your contribution :)

Updated this patch to treat epoch time as seconds not milliseconds. Am I correct in assuming that the :time of a Mirabelle event is in epoch seconds, like a Riemann event?

The :time value can have different types:

I/O should usually support all of these types (and not throw if a ratio or a double is passed for example). For example, in the InfluxDB I/O, I do (long (* 1000000 (:time event))) to not lose precision and to be sure I have a long at the end: https://github.com/mcorbin/mirabelle/blob/master/src/clojure/mirabelle/io/influxdb.clj#L67

Thank you for the fix about the local time zone. I was also indeed calling long twice.

ofEpochSeconds works as expected for all types, so I think we are good with your modifications:

(java.time.Instant/ofEpochSecond (/ 19 3))
#object[java.time.Instant 0x3445dce0 "1970-01-01T00:00:06Z"]
mirabelle.time> (java.time.Instant/ofEpochSecond 19)
#object[java.time.Instant 0x55f7b836 "1970-01-01T00:00:19Z"]
mirabelle.time> (java.time.Instant/ofEpochSecond 19.10)
#object[java.time.Instant 0x6da8e2bc "1970-01-01T00:00:19Z"]

I also have no idea how to check this timestamp value in the Pagerduty UI to be honest, only the creation time seems displayed :D

(Superb documentation, by the way). <3

Thank you, don't hesitate to give me feedbacks or your thoughts about the project and what can be improved or added. I should really configure Github action on the repo for example (I will do it this week end).

If it's OK for you I can merge the PR.

ninaspitfire commented 3 years ago

Hi,

Thank you for all this wonderful explanation!

So if time_micros exists on the Protobuf message, :time will be seconds as float64 / double. If only time is on the message, :time will be seconds as int64 / long. Is that correct? I got a bit confused because the old tests were asserting that the ISO timestamp should be 1 millisecond after the epoch.

ofEpochSeconds works as expected for all types, so I think we are good with your modifications

Thanks. I tried it in the REPL with long and float, but not a rational. I can add tests for the different types, if you like.

I also have no idea how to check this timestamp value in the Pagerduty UI to be honest, only the creation time seems displayed :D

I spend more time in the PagerDuty interface than I would like to admit! I'll have a play around :). In my Riemann config, I add the :time and the time the incident was triggered as ISO-8601 timestamps in :custom_details.

EDIT: I can't find anything useful that PagerDuty does with the top-level timestamp, either. :confused:

Thank you once again for being so helpful and open to discussion.

mcorbin commented 3 years ago

So if time_micros exists on the Protobuf message, :time will be seconds as float64 / double. If only time is on the message, :time will be seconds as int64 / long. Is that correct?

Yes

I will merge the pull request, I tested ratios locally and it also works.

EDIT: I can't find anything useful that PagerDuty does with the top-level timestamp, either. confused

Same for me :D

mcorbin commented 3 years ago

Thank you again for your contribution.