Fix to_utc_datetime (?) #3899

ruslandoga · 2024-03-15T01:54:16Z

Changes

I'm not sure if the tests use some special date but it seems like the conversion was wrong as Santiago is not the same as GMT.

I noticed this problem in https://github.com/plausible/analytics/actions/runs/8289969596/job/22687289421?pr=3898

Tests

Automated tests have been added

Changelog

Entry has been added to changelog

Documentation

This change does not need a documentation update

Dark mode

This PR does not change the UI

ruslandoga · 2024-03-15T02:01:27Z

lib/plausible/timezones.ex


-      %Timex.AmbiguousDateTime{after: after_dt} ->
-        Timex.Timezone.convert(after_dt, "UTC")
+      {:gap, _before_dt, after_dt} ->


In some cases before dt might be preferred. Like maybe in where: ^to_utc_datetime(naive, site.timezone) <= timestamp so I've added a third argument :up | :down which defaults to :up (current behaviour)

aerosol · 2024-03-15T07:59:32Z

test/plausible/timezones_test.exs

@@ -23,7 +23,7 @@ defmodule Plausible.TimezonesTest do
    assert to_utc_datetime(~N[2022-09-11 00:00:00], "Etc/UTC") == ~U[2022-09-11 00:00:00Z]

    assert to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") ==
-             ~U[2022-09-11 00:00:00Z]
+             ~U[2022-09-11 04:00:00Z]


Hey, thanks for spotting it @ruslandoga.

On CI the failure was different:

left: ~U[2022-09-11 03:00:00Z] right: ~U[2022-09-11 00:00:00Z]

As much as I'm in favor of using stdlib over Timex, I need better clarity on what happened and if we're doing the right thing here. Why has the test started failing? Was it updated TZ DB or DST change or something else entirely? Is stdlib using up to date information?

My confidence/knowledge about timezones is lacking unfortunately.

This was untested before 518cdb3.

America/Santiago was used here because it was problematic in a certain case - see 5c8f39a.

I expect more issues the coming days as lots of DSTs happen in March?

Paging @zoldar too

Why has the test started failing?

My guess at the time of this PR was that I started using a new actions@cache key. Now I'm not sure.

I'll need to read up on those PRs and check Timex logic to answer the rest.

On CI the failure was different:

Yeah, on CI it keeps switching between 03:00 and 04:00

I don't know the reason yet. But I'd like to blame tzdata.

There was also #3873 done recently (and reverted via #3885)

Yeah, saw that one: #3811 (comment)

Was it updated TZ DB or DST change or something else entirely?

Seems like a bug in Timex.

master

assert to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") == ~U[2022-09-11 00:00:00Z]

Timex uses Tzdata in a way that doesn't resolve America/Santiago and Plausible falls back to UTC

Plausible.Timezones.to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") # {:error, {:could_not_resolve_timezone, "America/Santiago", 63830073600, :wall}} Timex.Protocol.NaiveDateTime.to_datetime(~N[2022-09-11 00:00:00], "America/Santiago") # {:error, {:could_not_resolve_timezone, "America/Santiago", 63830073600, :wall}} Timex.Timezone.convert(~N[2022-09-11 00:00:00], "America/Santiago") # {:error, {:could_not_resolve_timezone, "America/Santiago", 63830073600, :wall}} Timex.Timezone.get("America/Santiago", ~N[2022-09-11 00:00:00]) # {:error, {:could_not_resolve_timezone, "America/Santiago", 63830073600, :wall}} Timex.Timezone.name_of("America/Santiago") # "America/Santiago" Tzdata.zone_exists?("America/Santiago") # true Tzdata.zone_list() # [...] Timex.Timezone.get_info("America/Santiago", ~N[2022-09-11 00:00:00], :wall) # {:error, {:could_not_resolve_timezone, "America/Santiago", 63830073600, :wall}} Timex.to_gregorian_seconds(~N[2022-09-11 00:00:00]) # 63830073600 Timex.Protocol.NaiveDateTime.to_gregorian_seconds(~N[2022-09-11 00:00:00]) # 63830073600 Tzdata.zone_exists?("America/Santiago") # true Tzdata.zone_list() # [...] Timex.Timezone.resolve("America/Santiago", 63830073600, :wall) # {:error, {:could_not_resolve_timezone, "America/Santiago", 63830073600, :wall}} Tzdata.periods_for_time("America/Santiago", 63830073600, :wall) # [] Tzdata.possible_periods_for_zone_and_time("America/Santiago", 63830073600, :wall) # {:ok, []} Tzdata.do_consecutive_matching([], match_fn, [], false) # []

Here nesting corresponds to position in the call stack. The calls go from top to bottom. After comment is the return value. The returned values (after #) go in the reverse direction (up).

The important function call that makes everything fail is Timex.Timezone.resolve. It doesn't check for gaps when Tzdata.periods_for_time("America/Santiago", 63830073600, :wall) returns []

fix-timezones

assert to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") == ~U[2022-09-11 04:00:00Z]

DateTime uses Tzdata (Timex just relays to it in https://github.com/bitwalker/timex/blob/7f584cef5794a5eac320ba957d17370849b0b212/lib/timezone/database.ex#L37) differently and resolves the timezone. It checks for gaps when Tzdata.periods_for_time("America/Santiago", 63830073600, :wall) returns []

Plausible.Timezones.to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") DateTime.from_naive(~N[2022-09-11 00:00:00], "America/Santiago") Calendar.get_time_zone_database() # Timex.Timezone.Database DateTime.from_naive(~N[2022-09-11 00:00:00], "America/Santiago", Timex.Timezone.Database) # {:gap, #DateTime<2022-09-10 23:59:59.999999-04:00 -04 America/Santiago>, #DateTime<2022-09-11 01:00:00-03:00 -03 America/Santiago>} Tzdata.TimeZoneDatabase.time_zone_periods_from_wall_datetime(~N[2022-09-11 00:00:00], "America/Santiago") # {:gap, {%{zone_abbr: "-04", utc_offset: -14400, std_offset: 0, until_wall: ~N[2022-09-11 00:00:00], from_wall: ~N[2022-04-02 23:00:00]}, ~N[2022-09-11 00:00:00]}, {%{zone_abbr: "-03", utc_offset: -14400, std_offset: 3600, until_wall: ~N[2023-04-02 00:00:00], from_wall: ~N[2022-09-11 01:00:00]}, ~N[2022-09-11 01:00:00]}} Tzdata.periods_for_time("America/Santiago", 63830073600, :wall) # [] Tzdata.TimeZoneDatabase.gap_for_time_zone("America/Santiago", 63830073600) # {:gap, {%{zone_abbr: "-04", utc_offset: -14400, std_offset: 0, until_wall: ~N[2022-09-11 00:00:00], from_wall: ~N[2022-04-02 23:00:00]}, ~N[2022-09-11 00:00:00]}, {%{zone_abbr: "-03", utc_offset: -14400, std_offset: 3600, until_wall: ~N[2023-04-02 00:00:00], from_wall: ~N[2022-09-11 01:00:00]}, ~N[2022-09-11 01:00:00]}} Tzdata.periods("America/Santiago") # {:ok, [...]} DateTime.from_naive_with_period(~N[2022-09-10 23:59:59.999999], "America/Santiago", %{zone_abbr: "-04", utc_offset: -14400, std_offset: 0, until_wall: ~N[2022-09-11 00:00:00], from_wall: ~N[2022-04-02 23:00:00]}) # #DateTime<2022-09-10 23:59:59.999999-04:00 -04 America/Santiago> DateTime.from_naive_with_period(~N[2022-09-11 01:00:00], "America/Santiago", %{zone_abbr: "-03", utc_offset: -14400, std_offset: 3600, until_wall: ~N[2023-04-02 00:00:00], from_wall: ~N[2022-09-11 01:00:00]}) #DateTime<2022-09-11 01:00:00-03:00 -03 America/Santiago> DateTime.shift_zone!(#DateTime<2022-09-11 01:00:00-03:00 -03 America/Santiago>, "Etc/UTC", Timex.Timezone.Database) # etc.

Timex is not checking for gaps when doing timezone conversion. This means all "winter time to summer time" gaps would be unhandled. Timex does handle "summer to winter" (ambiguous) time however.

Why has the test started failing?

tzdata's fault ;)

master

iex> Tzdata.tzdata_version() "2021e" # this version didn't include gap for Santiago on 2022-09-11 (probably cause it's in the future?) iex> Plausible.Timezones.to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") ~U[2022-09-11 03:00:00Z] iex> Tzdata.DataLoader.download_new() {:ok, 451270, "2024a", "/var/folders/cr/fjw4jzx53ybbrl325z6nwd6h0000gn/T/tzdata_data/tmp_downloads/451270_80870041/", "Thu, 01 Feb 2024 18:40:48 GMT"} iex> Tzdata.DataBuilder.load_and_save_table() {:ok, 451270, "2024a"} iex> Tzdata.EtsHolder.new_release_has_been_downloaded() :ok iex> Tzdata.tzdata_version() "2024a" iex> Plausible.Timezones.to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") ~U[2022-09-11 00:00:00Z]

So depending on when the tests for timezones run, tzdata might or might have not had enough time to update the database. If the version is 2021e, the tests fail with ~U[2022-09-11 03:00:00Z] != ~U[2022-09-11 00:00:00Z]. It showed up in my ci branch since I started using a new cache key and tzdata wasn't cached.

#3811 is also partly at fault since it started using System.tmp_dir! for storing tzdata stuff which is not cached in CI, that causes the flipping in #3899 (comment)

fix-timezones

iex> Tzdata.tzdata_version() "2021e" iex> Plausible.Timezones.to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") ~U[2022-09-11 03:00:00Z] iex> Tzdata.DataLoader.download_new() {:ok, 451270, "2024a", "/var/folders/cr/fjw4jzx53ybbrl325z6nwd6h0000gn/T/tzdata_data/tmp_downloads/451270_80870041/", "Thu, 01 Feb 2024 18:40:48 GMT"} iex> Tzdata.DataBuilder.load_and_save_table() {:ok, 451270, "2024a"} iex> Tzdata.EtsHolder.new_release_has_been_downloaded() :ok iex> Tzdata.tzdata_version() "2024a" iex> Plausible.Timezones.to_utc_datetime(~N[2022-09-11 00:00:00], "America/Santiago") ~U[2022-09-11 04:00:00Z]

Fixed in #3898

ruslandoga · 2024-03-16T01:53:25Z

lib/plausible/timezones.ex

+  Examples:
+
+      iex> to_datetime_in_timezone(~U[2024-03-16 01:50:45.180928Z], "Asia/Kuala_Lumpur")
+      #DateTime<2024-03-16 09:50:45.180928+08:00 +08 Asia/Kuala_Lumpur>


$ date Sat Mar 16 09:50:49 CST 2024

iex(1)> DateTime.utc_now ~U[2024-03-16 01:50:45.180928Z]

Previously:

iex> Tzdata.TimeZoneDatabase.time_zone_periods_from_wall_datetime(~U[2024-03-16 01:50:45.180928Z], "Asia/Kuala_Lumpur") {:ok, %{ zone_abbr: "+08", utc_offset: 28800, std_offset: 0, until_wall: :max, from_wall: ~N[1982-01-01 00:00:00] }} iex> Tzdata.TimeZoneDatabase.time_zone_periods_from_wall_datetime(~U[2024-03-16 01:50:45.180928Z], "Etc/GMT-8") {:ok, %{ zone_abbr: "+08", utc_offset: 28800, std_offset: 0, until_wall: :max, from_wall: :min }} iex> Plausible.Timezones.to_datetime_in_timezone(~N[2024-03-16 01:50:45.180928], "Etc/GMT-8") #DateTime<2024-03-15 17:50:45.180928-08:00 -08 Etc/GMT+8>

I'm not sure what's happening here exactly. Why is the new timezone +8 and not -8? It seems to have gone in the other direction. I don't know if that's how GMT+X timezones are supposed to work though.

This PR:

iex> Plausible.Timezones.to_datetime_in_timezone(~N[2024-03-16 01:50:45.180928], "Etc/GMT-8") #DateTime<2024-03-16 09:50:45.180928+08:00 +08 Etc/GMT-8>

I think this makes more sense since Tzdata shows that Etc/GMT-8 and Asia/Kuala_Lumpur timezones are equivalent for that point in time.

Related: bitwalker/timex#685

Seems like Etc/GMT timezones are indeed supposed to be inverted:

https://chat.openai.com/share/2815f806-54e5-4758-8c06-d63e6f31a0a1

https://stackoverflow.com/questions/53076575/time-zones-etc-gmt-why-it-is-other-way-round

~~So Timex is right!~~

Ah no, it seems to be wrong still... Or maybe Timex is right and everything else is wrong :)

ruslandoga · 2024-03-16T04:39:09Z

lib/plausible/sites.ex


  ## Examples

    iex> Plausible.Site.local_start_date(%Plausible.Site{stats_start_date: nil})
    nil

-    iex> utc_start = ~N[2022-09-28 00:00:00]


site.starts_start_date is a date

ruslandoga · 2024-03-16T04:41:45Z

lib/plausible/sites.ex

    iex> Plausible.Site.local_start_date(site)
    ~D[2022-09-27]
+
  """
  @spec local_start_date(Site.t()) :: Date.t() | nil
  def local_start_date(site) do


I'm not sure this function is necessary. stats_start_date is already local thanks to

analytics/lib/plausible/stats/clickhouse.ex

Line 29 in d3586a8

Timezones.to_date_in_timezone(datetime, site.timezone)

and imports are supposed to be ~~UTC #3895 (comment)~~ local time https://3.basecamp.com/5308029/buckets/35871979/card_tables/cards/7202028179#__recording_7227026473 as well. Either way, shifting timezones on a date feels wrong somehow.

But I guess it doesn't hurt since there is no data a day before the start date.

ruslandoga requested a review from a team March 15, 2024 01:54

ruslandoga commented Mar 15, 2024

View reviewed changes

fix timezones

d88722a

ruslandoga force-pushed the fix-timezones branch from 57258c6 to d88722a Compare March 15, 2024 02:02

ruslandoga changed the title ~~Fix timezones (?)~~ Fix to_utc_datetime (?) Mar 15, 2024

ruslandoga mentioned this pull request Mar 15, 2024

Split Elixir CI jobs #3898

Merged

4 tasks

aerosol reviewed Mar 15, 2024

View reviewed changes

ruslandoga mentioned this pull request Mar 15, 2024

Split elixir tests no tz fix #3902

Closed

ruslandoga requested a review from aerosol March 15, 2024 11:42

shift in the opposite direction in to_datetime_in_timezone

bbdf897

ruslandoga commented Mar 16, 2024

View reviewed changes

ruslandoga added 2 commits March 16, 2024 10:07

more tests

0be28cf

eh

5e67f39

ruslandoga commented Mar 16, 2024

View reviewed changes

ruslandoga added 5 commits March 16, 2024 12:55

eh

ddbb79f

eh

3bbf28f

eh

98d4b7b

cleanup

f8e53b5

improve tests/docs

5844a84

ruslandoga mentioned this pull request Mar 19, 2024

CSV imports (no UI) #3895

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to_utc_datetime (?) #3899

Fix to_utc_datetime (?) #3899

ruslandoga commented Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

aerosol Mar 15, 2024

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

aerosol Mar 15, 2024

ruslandoga Mar 15, 2024

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

Fix to_utc_datetime (?) #3899

Are you sure you want to change the base?

Fix to_utc_datetime (?) #3899

Conversation

ruslandoga commented Mar 15, 2024 • edited

Changes

Tests

Changelog

Documentation

Dark mode

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

aerosol Mar 15, 2024

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

aerosol Mar 15, 2024

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 15, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024 • edited

Choose a reason for hiding this comment

ruslandoga Mar 16, 2024 • edited

Choose a reason for hiding this comment

ruslandoga commented Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 15, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited

ruslandoga Mar 16, 2024 •

edited