-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Make comparisons to today consider only stats in the same exact time range #6205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
abe385d
f95c32d
8db21bc
6d53131
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,8 @@ defmodule Plausible.Stats.Comparisons do | |
| """ | ||
|
|
||
| alias Plausible.Stats | ||
| alias Plausible.Stats.{Query, DateTimeRange, Time} | ||
| alias Plausible.Stats.{DateTimeRange, Query, Time} | ||
| alias Plausible.Times | ||
|
|
||
| @spec get_comparison_utc_time_range(Stats.Query.t()) :: DateTimeRange.t() | ||
| @doc """ | ||
|
|
@@ -53,8 +54,8 @@ defmodule Plausible.Stats.Comparisons do | |
| DateTimeRange.new!(from, to) | ||
|
|
||
| _ -> | ||
| # For 24h period, work directly with datetime ranges to preserve time precision | ||
| if source_query.input_date_range == :"24h" do | ||
| # For 24h period and today, work directly with datetime ranges to preserve time precision | ||
| if use_datetime_for_comparison?(source_query) do | ||
| get_comparison_datetime_range(source_query) | ||
| else | ||
| comparison_date_range = get_comparison_date_range(source_query) | ||
|
|
@@ -70,6 +71,17 @@ defmodule Plausible.Stats.Comparisons do | |
| DateTimeRange.to_timezone(datetime_range, "Etc/UTC") | ||
| end | ||
|
|
||
| defp use_datetime_for_comparison?(query) do | ||
| if query.input_date_range == :day do | ||
| today_from_now = Times.to_date(query.now, query.timezone) | ||
| day_from_range = Times.to_date(query.utc_time_range.first, query.timezone) | ||
|
|
||
| Date.compare(today_from_now, day_from_range) == :eq | ||
| else | ||
| query.input_date_range == :"24h" | ||
| end | ||
| end | ||
|
|
||
| def get_comparison_query( | ||
| %Query{comparison_utc_time_range: %DateTimeRange{} = comparison_range} = source_query | ||
| ) do | ||
|
|
@@ -120,38 +132,34 @@ defmodule Plausible.Stats.Comparisons do | |
| end | ||
| end | ||
|
|
||
| # For 24h period, shift the datetime range directly to preserve time precision | ||
| # For 24h and today periods, shift the datetime range directly to preserve time precision | ||
| defp get_comparison_datetime_range( | ||
| %Query{ | ||
| input_date_range: :"24h", | ||
| include: %{compare: :previous_period, compare_match_day_of_week: true} | ||
| input_date_range: input_range, | ||
|
zoldar marked this conversation as resolved.
|
||
| include: %{compare: :previous_period} = include | ||
| } = source_query | ||
| ) do | ||
| days_back = 7 | ||
| comparison_start = DateTime.shift(source_query.utc_time_range.first, day: -days_back) | ||
| comparison_end = DateTime.shift(source_query.utc_time_range.last, day: -days_back) | ||
|
|
||
| DateTimeRange.new!(comparison_start, comparison_end) | ||
| end | ||
| ) | ||
| when input_range in [:"24h", :day] do | ||
| offset = | ||
| if include.compare_match_day_of_week do | ||
| [day: -7] | ||
| else | ||
| [hour: -24] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question, non-blocking: does -24h work as expected during changeover from DST?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what's expected, but AFAIK the conversion happens later on via |
||
| end | ||
|
|
||
| defp get_comparison_datetime_range( | ||
| %Query{ | ||
| input_date_range: :"24h", | ||
| include: %{compare: :previous_period} | ||
| } = source_query | ||
| ) do | ||
| comparison_start = DateTime.shift(source_query.utc_time_range.first, hour: -24) | ||
| comparison_end = DateTime.shift(source_query.utc_time_range.last, hour: -24) | ||
| comparison_start = DateTime.shift(source_query.utc_time_range.first, offset) | ||
| comparison_end = DateTime.shift(source_query.utc_time_range.last, offset) | ||
|
|
||
| DateTimeRange.new!(comparison_start, comparison_end) | ||
| end | ||
|
|
||
| defp get_comparison_datetime_range( | ||
| %Query{ | ||
| input_date_range: :"24h", | ||
| input_date_range: input_range, | ||
| include: %{compare: :year_over_year} | ||
| } = source_query | ||
| ) do | ||
| ) | ||
| when input_range in [:"24h", :day] do | ||
| comparison_start = DateTime.shift(source_query.utc_time_range.first, year: -1) | ||
| comparison_end = DateTime.shift(source_query.utc_time_range.last, year: -1) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question, non-blocking: Did you consider creating a new :today period and remapping :day to it when appropriate? Because "today" gets special treatment, it might lead to clearer code overall. Probably more code though :D
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not. However, looking at it now, I think adding this period would add a lot more complexity in other parts of logic working with the query. Also, we'd have to probably maintain support for the "old" way of defining period to support existing external links. Not worth it IMHO.