fix(template): always marshal Alerts time to JSON with time-secfrac part

[trearcul]

Fixes behavior of marshalling time.Time type to JSON which completely omits milliseconds when time.Time object contains zero milliseconds.
The default marshalling behavior can cause issues during deserialization of sent webhook notifications.

Example:
Alert.StartsAt is equal to time.Date(2023, 11, 14, 15, 49, 10, 0, time.UTC)

Expected JSON value: 2023-11-14T15:49:10.000Z
Actual value: 2023-11-14T15:49:10Z

[grobinson-grafana]

Hi! 👋 I'm afraid this doesn't look like a bug to me. Go uses RFC3339 when representing time in JSON, and in the specification milliseconds are optional as the definition of partial-time has an optional time-secfrac (see here):

partial-time    = time-hour ":" time-minute ":" time-second
                  [time-secfrac]

It sounds like the software you are using to deserialize webhooks might not be RFC3339 compliant?

[trearcul]

Hi George, thank you for your reply. I'm aware of the RFC3339 and you are totally right. I'm sorry for wrongly marking the PR as a "fix". Nevertheless, I still believe having a fixed datetime format is worth because:

In Alertmanager:

• Narrows down number of possible JSON formats for datetime in the Alertmanager
  • Hook notifications uses time.Time type
  • API v2 /alerts endpoint already uses strfmt.DateTime type which always marshals secfrac to JSON
• Simplifies deserialization of sent webhook notifications in notified systems where developer wants to be strict about datetime input format

In general

• Predictable and always demonstrates system's secfrac precision which the RFC keeps open for implementation.
• Improves DX and doesn't break the RFC compliance

PS: I've always thought that optionality of secfrac part in the RFC is meant for systems and contexts which doesn't utilize such precision at all

[grobinson-grafana]

Hi! 👋 I think the decision of whether or not to force millisecond precision in notifications will have to be made by the maintainers. However, a strong argument for making this change is that notifications are then consistent with the Alertmanager APIs, as you said.

To reply to your points:

1. I believe there is just one date time format in Alertmanager and that is RFC3339. A timestamp without millisecond precision is still RFC3339. These are the same formats.
2. You are right that Alertmanager APIs force millisecond precision. This seems to be an artifact of using OpenAPI code generation which uses RFC3339 with millisecond precision (i.e. time-secfrac). It seems like OpenAPI does this to avoid issues with clients and APIs that don't implement the full ISO8601/RFC3339 specification (https://github.com/go-openapi/strfmt/blob/master/time.go#L119-L120)?
3. I'd encourage you to nonetheless to make sure all your system(s) that receives notifications from Alertmanager fully understand timestamps in RFC3339. For example, please don't use a regular expression that checks there are 3 digits of precision for milliseconds. Alertmanger could nanosecond precisions in future. Use a fully-compliant RFC3339 parser instead.
4. Alertmanager notifications at present contain nanosecond precision, so we would lose some precision with this change. I'm not sure if there are people out there that rely on having nanosecond precision, but it's something for the maintainers to keep in mind when reviewing this change.

[trearcul]

Hi George, thank you for your reply.

Regarding your replies to my points
(1) That's the point where I disagree, but I'm not gonna argue about whether the RFC3339 is a format or a standard defining multiple formats (date, time, time with secfrac, datetime, datetime with secfrac). IMHO it's just a matter of perspective.
(3) I use a client that is fortunately fully RFC3339 compliant, and I really don't check for secfrac exactly 3 digits long 😄
(4) Yep, you are right. That's a mistake I made. It should be formatted with nanosecs.

The core idea of this PR is in fact written in RFC3339 itself in section 5.3. Rarely Used Options suggesting the inclusion of time-secfrac based on usage (either always or never) and not by a timestamp value.

A format which includes rarely used options is likely to cause interoperability problems. This is because rarely used options are less likely to be used in alpha or beta testing, so bugs in parsing are less likely to be discovered. Rarely used options should be made mandatory or omitted for the sake of interoperability whenever possible.

The format defined below includes only one rarely used option: fractions of a second. It is expected that this will be used only by applications which require strict ordering of date/time stamps or which have an unusual precision requirement.

[grobinson-grafana]

I think Section 5.3 of RFC3339 is also a good argument to do this. I'm not sure if swagger codegen can be configured to use nanosecond precision, so I think either Alertmanager should use millisecond precision in notifications (at the loss of some precision) of we figure out how to add nanosecond precision to the swagger codegen.