Implement "Year over Year" comparison mode #2704
Conversation
BundleMonFiles updated (1)
Unchanged files (6)
Total files change +195B +0.02% Final result: ✅ View report in BundleMon website ➡️ |
|
|
||
| const COMPARISON_MODES = { | ||
| 'previous_period': 'Previous period', | ||
| 'last_year': 'Last year', |
There was a problem hiding this comment.
I'd love some feedback on what we should call this comparison mode. I named it "Last year", but it's pretty common to call this "Year over year" too. Essentially it compares the current stats with last year stats, e.g. Jan 2021 compared to Jan 2020.
There was a problem hiding this comment.
nice! "Previous period" and "Year over year" are good i think
There was a problem hiding this comment.
I think I prefer 'year over year' vs 'last year'
| let comparison = !!q.get('comparison') | ||
| let comparison = q.get('comparison') |
There was a problem hiding this comment.
Previously the comparison query property was a boolean, on or off. Now it supports multiple modes and it's a string, e.g. last_year, previous_period.
vinibrsl
force-pushed
the
year-over-year
branch
3 times, most recently
from
808b4af
to
16801c0
Compare
|
|
||
| const COMPARISON_MODES = { | ||
| 'previous_period': 'Previous period', | ||
| 'last_year': 'Last year', |
There was a problem hiding this comment.
I think I prefer 'year over year' vs 'last year'
| comparison_query = Query.shift_back(query, site) | ||
| Stats.timeseries(site, comparison_query, [selected_metric]) | ||
|
|
||
| "last_year" -> |
There was a problem hiding this comment.
Hmm, I think this implementation is missing the incomplete period logic that we have in Query.shift_back.
The idea is that comparisons should compare the same number of months/weeks/days/hours. For example, if you're in the middle of the month and looking at 'this month', then we display the full month even though there's only data for the first 14 days. In this case we want the comparison to also only include 14 days from the comparison period.
We've started to add clauses to shift_back to account for this discrepancy. I think some are still missing, and that's what this PR is about.
I think similar special cases need to be added for the year over year comparison mode. When 'this month' only includes 14 days, we need to shift back one year but also make sure we only select 14 days to compare with.
Does that make sense? Happy to hop on a quick call to dive deeper
There was a problem hiding this comment.
Good catch! If I understand that correctly, we should not show comparisons for any future date in the graph, is that it? Even if comparison has data to show.
We could reimplement what we have for shift_back to match the comparison logic. Or we could truncate the comparison plot to match the same number of items of the main plot. What do you think?
I'm up for pairing on this. We can talk more about it tomorrow in our call.
There was a problem hiding this comment.
Right, yes. In your screenshot 'month to date' view shows 30 days and compares with 30 days from the previous year. However, since there has only been 1 day this March it's not a fair comparison. The comparison period should also be just 1 day from the previous March.
This applies for any time period that can show an incomplete period: today, this month and year to date come to mind. We already handle period: month and period: year correctly. We don't currently handle today correctly which is addressed in this PR.
We could reimplement what we have for shift_back to match the comparison logic. Or we could truncate the comparison plot to match the same number of items of the main plot. What do you think?
I think it needs to be in shift_back. If we only truncate the comparison plot then the percentages in top_stats will still be off.
| start_date = Date.add(source_query.date_range.first, -365) | ||
| end_date = Date.add(source_query.date_range.last, -365) | ||
| range = Date.range(start_date, end_date) | ||
| end_date = earliest(source_query.date_range.last, now) |> Date.add(-365) |
vinibrsl
changed the title
| case Stats.Comparisons.compare(site, query, comparison_mode) do | ||
| {:ok, prev_query} -> | ||
| %{visitors: %{value: prev_converted_visitors}, events: %{value: prev_completions}} = | ||
| Stats.aggregate(site, prev_query, [:visitors, :events]) |
There was a problem hiding this comment.
Not a concern for this PR but I think eventually we want to also support the different comparison modes in the API. In that case it would make sense to push the comparison logic down to Stats.Aggregate and Stats.Timeseries so the logic can be shared between the dashboard and the API.
Changes
This pull request adds support for multiple comparison modes, changes the comparison checkbox to a combobox, and implements the year over year / last year comparison mode. The feature is still behind a feature flag.
Tests
Changelog
Documentation
Dark mode