Improve performance of evolution graphs when viewing large date ranges or comparing dates and improve ability to see trends #20100
Conversation
| $highestPeriodInCommon = $this->getHighestPeriodInCommon($originalPeriod, $comparisonPeriods); | ||
|
|
||
| if ($isComparingDates) { | ||
| if ($isComparing) { |
There was a problem hiding this comment.
When comparing periods, for consistency both sparklines and evolution graphs will now behave the same and show the evolution over the selected range instead of sparklines showing historical evolution.
|
|
||
| // if a larger date range is selected, then for better performance and for seeing trends better we want to use | ||
| // a suitable period (rather than always using for example the day range) | ||
| $this->requestConfig->request_parameters_to_modify['period'] = $selector->getHighestPeriodInCommon($requestingPeriod, []); |
There was a problem hiding this comment.
When now viewing a large range, the evolution graph may switch from day period to week or month or year to needing to fetch less archives for faster performance and it allows detecting trends better
| } | ||
|
|
||
| $this->config->custom_parameters['columns'] = $this->config->columns_to_display; | ||
|
|
||
| if ($this->isComparing()) { | ||
| $this->config->show_limit_control = false; // since we always show the days over the period, there's no point in changing the limit | ||
| $this->config->show_limit_control = false; // since we always show the evolution over the period, there's no point in changing the limit | ||
| $this->config->show_periods = false; // the periods can't be changed as they are always fixed when comparing |
There was a problem hiding this comment.
Disabling selecting period in evolution graph when comparison feature is used as it was never working.
| } | ||
|
|
||
| $this->requestConfig->request_parameters_to_modify = $selector->setDatePeriods( |
There was a problem hiding this comment.
When now comparing date ranges, the evolution graph may switch from day period to week or month or year to needing to fetch less archives.
This now ensures sparklines and evolution graphs use the same periods when comparing.
| { | ||
| $isComparingDates = !empty($comparisonPeriods); | ||
| $highestPeriodInCommon = $this->getHighestPeriodInCommon($originalPeriod, $comparisonPeriods); |
There was a problem hiding this comment.
This one always seems to return the highest period of the shortest comparison periods.
So if someone would e.g. compare 2022-08-09;2022-08-10 with 2016-08-09;2022-08-10, it would try to use the day period for both. This might still lead to a problem, as the very long second period would be fetched as days.
Not needed to improve that in this PR as well. But was just wondering if we should try to improve that in some way.
We could e.g. restrict that compared periods are more than twice longer than the shortest compared period or something similar.
There was a problem hiding this comment.
Good point. Indeed it's not ideal. Put it in there for now on purpose so at least both use the same period and it's comparable. With some of the other changes coming it'll be at least a lot faster and not sure if it's done a lot in practice as it wouldn't make all that much sense to compare these periods but can always happen when they select wrong year for example by accident.
|
Thanks @sgiehl 👍 |
justinvelluppillai
added
the
not-in-changelog
Description:
Since merging #20094 and #20090 sparklines load a lot faster and the comparison feature is now usable for most dates. However, when comparing for example two years then evolution graph is still slow and sometimes after minutes of loading even times out.
This change now uses the same logic to also speed up evolution graphs and makes sure that both sparklines and evolution graphs use the same periods.
Besides this we're now also:
Review