Add timezone support to time intervals.

[benridley]

This PR addresses #2771 by adding a time_zone field to time interval definitions, which accepts an IANA time zone name (like 'Australia/Sydney') and if supplied means that the time intervals are defined inside that time zone.

On Windows machines the only time zones that are accepted are 'Local' and 'UTC', but I think that is probably enough for 99% of users who just want to use their local server time zone. I have noted this in the documentation, and also added some more info to the error message when a timezone fails to load on Windows mentioning that the user may need to supply their own time zone database.

I think it's also probably worth changing the default to be the server's local time rather than UTC as this would probably be unexpected for most users, but it is breaking behavior so I'm not sure how we should proceed with that but open to the ideas.

Feedback on the approach (particularly dealing with Windows) is more than welcome. Cheers!

[benridley]

It seems I've fixed the tests that were failing, but I'm not sure what's going wrong with the 'mixin' part of the CI pipeline, it looks unrelated to my changes...

[philipgough]

@benridley looks like this was addressed in #2785 so a rebase on top of main should resolve this for you

[benridley]

Thanks @philipgough, I've just rebased so we'll see how it goes.

[simonpasquier]

The time zone will be used to You may also use 'Local'

this doesn't read correctly?

[simonpasquier]

I'm wondering if we could make the error message more targeted.

Suggested change

	if runtime.GOOS == "windows" && str != "Local" && str != "UTC" {
	return fmt.Errorf("unable to load time location. Timezones other than 'Local' and 'UTC' may not be supported on Windows without using a custom timezone file: %w", err)
	}
	return err
	if runtime.GOOS == "windows" {
	if zoneinfo := os.GetEnv("ZONEINFO"); zoneinfo != "" {
	return fmt.Errorf("%w (ZONEINFO=%q)", err, zoneinfo)
	}
	return fmt.Errorf("%w (on Windows platforms, you may have to pass the time zone database using the ZONEINFO environment variable, see https://pkg.go.dev/time#LoadLocation for details)", err)
	}
	return err

[benridley]

Thanks @simonpasquier.

Yeah I think that's a good idea. I've also been thinking more, and I'm wondering if we just go the extra distance and embed the timezone database on Windows platforms? Obviously it's not ideal because we can have stale timezone information between releases of AM, but I think that's probably better than nothing. What do you think?

[simonpasquier]

I would defer it for a later PR if people feel that it's really needed. As you said, we're at risk of providing stale information.

[beorn7]

As discussed elsewhere, while "timezone" in the context of the TZ database indeed refers to a location (e.g. "Australia/Sydney"), the general understanding of "timezone" is more ambiguous. In many other contexts, a timezone is something like "PDT" or "PST" (or even "PST/PDT" 😱 ). As a concrete example particularly relevant for Go developers, look at the terminology in https://pkg.go.dev/time#Time .

Therefore, I suggest to use a word like "location". Even if you come from a TZ database context, you will still understand what that is, while those from other contexts will have a much easier life (not trying to enter "PDT" and wondering why it doesn't work).

[benridley]

Sure @beorn7, I think that's a good suggestion and keeps the terminology consistent with Go's stdlib. Happy to add this.

[simonpasquier]

Great feedback from @beorn7 (as usual) on location vs. time zone, +1 for me.

[piotr1212]

Hey, wondering if this is still being worked on as we'd need this feature. Also assuming this works well with daylight saving?

[benridley]

Hey @piotr1212,

Yeah it's coming - I was mainly waiting on #2779 because I knew I'd need to rebase and fix this code up once that was merged. I'll probably work on it this weekend and get it tidied up.

And yes, it will work fine with daylight savings - Provided you aren't running AM on Windows.

[benridley]

I've just rebased onto main, and also added in the following:

• Renamed Timezone/time_zone to Location/location as its a more accurate description and consistent with Go's stdlib (thanks @beorn7)
• Added in @simonpasquier's suggestion to be more specific with the Windows error and check.
• Fixed up the docs to use the 'location' keyword, and also added some more details about what it does.

[sylr]

@simonpasquier could we go ahead and merge this please ?

[benridley]

Thanks for those test corrections @sylr, I've just merged them.

[simonpasquier]

lgtm.

We could maybe add a blank import to https://pkg.go.dev/time/tzdata in cmd/alertmanager/main.go to embed a copy of the timezone database but it can be done later if we see fit.