Support realtime periods in API v2 #4469
Conversation
This commit starts parsing date ranges into a new NaiveDateTimeRange struct, rather than a simple Date.Range.
test/plausible_web/controllers/api/external_stats_controller/query_test.exs
Show resolved
Hide resolved
Co-authored-by: Karl-Aksel Puulmann <[email protected]>
Co-authored-by: Karl-Aksel Puulmann <[email protected]>
Co-authored-by: Karl-Aksel Puulmann <[email protected]>
Co-authored-by: Karl-Aksel Puulmann <[email protected]>
|
| def parse(site, schema_type, params, now \\ nil) when is_map(params) do | ||
| def parse(site, schema_type, params, test_opts \\ []) when is_map(params) do | ||
| now = Keyword.get(test_opts, :now, DateTime.utc_now(:second)) | ||
| date = Keyword.get(test_opts, :date, today(site)) |
There was a problem hiding this comment.
Q: Why do we need both date and now parameters? Isn't date = now |> to_date?
There was a problem hiding this comment.
You're right, we can work with only now. d3a9035
| with :ok <- JSONSchema.validate(schema_type, params), | ||
| {:ok, date} <- parse_date(site, Map.get(params, "date"), now), | ||
| now <- if(now, do: now, else: DateTime.utc_now(:second)), |
There was a problem hiding this comment.
I believe this if check can never fail right?
There was a problem hiding this comment.
Removed in d3a9035
| end | ||
|
|
||
| defp datetime_from_timestamp(timestamp_string) do | ||
| with [timestamp, timezone] <- String.split(timestamp_string), |
There was a problem hiding this comment.
Can we avoid cooking our encoding scheme and use DateTime.from_iso8601 instead?
There was a problem hiding this comment.
Unfortunately not. The ISO8601 format alone does not provide the timezone information which we need to create the DateTime struct.
| @@ -513,6 +612,121 @@ defmodule Plausible.Stats.Filters.QueryParserTest do | |||
|
|
|||
| %{"site_id" => site.domain, "date_range" => ["21415-00", "eee"], "metrics" => ["visitors"]} | |||
| |> check_error(site, "#/date_range: Invalid date range [\"21415-00\", \"eee\"]") | |||
|
|
|||
| # Timestamps do not include timezone information | |||
There was a problem hiding this comment.
There's too many test cases in this test case.
Let's create separate test cases.
|
|
||
| # Checks parsing the date range relative to the `date` parameter (which is only | ||
| # available in the internal API schema) instead of using `@now`. | ||
| def check_date_range_with_date(date_range, site, expected_date_range) do |
There was a problem hiding this comment.
Nit: Can we make date an optional parameter of check_date_range instead?
E.g. `def check_date_range(date_params, site, expected_date_range, schema_type \ public)
This reduces the number of abstractions we need to deal with reading tests.
| @@ -79,6 +81,15 @@ defmodule Plausible.Stats.QueryResult do | |||
| |> Enum.reject(fn {_, value} -> is_nil(value) end) | |||
| |> Enum.into(%{}) | |||
| end | |||
|
|
|||
| defp to_iso_8601_with_timezone(%DateTime{time_zone: timezone} = datetime) do | |||
There was a problem hiding this comment.
Q: would DateTime.to_iso8601(datetime, :extended) not work here? This should make the parsing more standard as well.
There was a problem hiding this comment.
This would include the UTC offset in the iso8601 string format, but not the timezone name that we need.
| case DateTime.new(last, ~T[23:59:59], timezone) do | ||
| {:ok, datetime} -> datetime | ||
| {:gap, just_before, _just_after} -> just_before | ||
| {:ambiguous, first_datetime, _second_datetime} -> first_datetime |
There was a problem hiding this comment.
Just curious: I wonder why lower values are picked for the end of the range, and higher -- for the beginning? This makes the whole range smaller: range=max(possible_timestamps)..min(possible_timestamps) - potentially leaving some events out. Why not range=min(possible_timestamps)..max(possible_timestamps)
It probably doesn't matter though, since clocks are usually moved at 02:00 or 03:00 so midnight should be safe from gaps and ambiguities.
There was a problem hiding this comment.
@ruslandoga, good question! The idea was to make sure that the date of the parsed datetime will always remain the same as the requested date.
One specific example: let's say the client requests a date_range of ["2022-09-11", "2022-09-11"] and their timezone is America/Santiago. The first datetime will be parsed as follows:
iex(3)> DateTime.new(~D[2022-09-11], ~T[00:00:00], "America/Santiago")
{: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>}
If we choose the first date (i.e. "just before"), we'll end up parsing the datetime into a different date ("2022-09-10") from what was actually requested ("2022-09-11").
That means, when dealing with the date range later on in code (i.e. constructing the time labels for timeseries requests or doing comparisons), we'd run into errors, or we'd have to build some ugly exceptions to handle these cases.
That said, when allowing a flexible datetime format in the public API, where one could request a date range such that 2022-09-11 00:00:00 in "America/Santiago" is the last datetime, we'd still run into the same issue.
So perhaps the proper fix could be to always select just_before or just_after depending on which falls on the same date? \cc @macobo also bringing this to your attention as you're working on it.
There was a problem hiding this comment.
Ah, I see. I didn't think about conversion to dates for labels and comparisons. I assumed the filtering always happens on "full" timestamps (i.e. DateTime). And I didn't know that there were timezones with gaps at midnight! Thank you for the explanation!
Changes
DateTimeRangestruct, including the timezone information.realtimeand30mdate range shortcuts in the private API JSON schemadateparameter (in the private API JSON schema) relative to which thedate_rangewill be constructed.Tests
Changelog
Documentation
Dark mode