Apply sanity caps on outlier values in performance reports

[rr-it]

Description:

The values used in performance reports of type 'sum/total' and 'average' are capped to reduce the impact of single failed performance measurements.

This fixes #17847.

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[sgiehl]

Hi @rr-it,
Thanks for your contribution. I had a look across the code changes and I'm not totally sure what they are meant for.
To confirm I've understood everything correct:

This PR introduces some sort of maximal values for the performance metrics that are used in calculations.
So if there were values tracked higher than the one configured, the calculations will use the configured values instead.

Is that correct?

[rr-it]

@sgiehl That's perfectly correct.

Please also see my latest comment on #17847:
#17847 (comment)

[rr-it]

Devices, especially iPhones, sometimes send insane high performance values which don't make any sense.
E.g. DOM processing time of 4 hours instead of 250 microseconds.

Without this PR

• 10000 values of 250 ms result in an average of 0.250 seconds
  $\frac{10000×250 ms}{10000} = 250 ms$
• One value of 4 h and 10000 values of 250 ms result in an average of ~ 1.690 seconds
  • $4h = 4×60×60×1000 ms = 14400000 ms$
  • $\frac{14400000 ms + 10000×250 ms}{10001} = 1690 ms$

With this PR

time_dom_processing_cap_duration_ms = 50000

• 10000 values of 250 ms result in an average of 0.250 seconds
  $\frac{10000×250 ms}{10000} = 250 ms$
• One value of 4 h and 10000 values of 250 ms result in an average of ~ 0.255 seconds
  • $4h = 4×60×60×1000 ms = 14400000 ms$
    $14400000 ms$ capped to $50000 ms$
  • $\frac{50000 ms + 10000×250 ms}{10001} = 255 ms$

[github-actions]

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

[rr-it]

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@sgiehl Please add label 'Do not close'.

[github-actions]

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

[sgiehl]

Left a comment. Otherwise the code looks fine to me.
But someone from product team should maybe have a look here first. ping @Stan-vw
I'm not 100% sure regarding the implementation/solution.
With cutting away values if they are too high I see some possible:

• Only removing too high values, but not also too low values might let look the values better than they are
• The default values for the config values might be inaccurate as some pages might expect higher loading times than others.
• The most accurate solution might be to simply throw away the top 1% and bottom 1% of values and only calculate with the rest. This would be a lot more complex to implement and maybe also slower when archiving, though.

If we plan to include that, guess we would need to update our documentation & faqs accordingly.

[Stan-vw]

I think we would like this report to not show the outliers (top & bottom 1% sounds like a fine test to begin with, but as you say that could be more implementation work maybe we can pick fixed values from historic reports).
However, I think we should still have a report that shows all values (if we don't have that already). I've attached some potential ways to visualise this, albeit that I didn't add the relevant axes (days vs seconds):

[github-actions]

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

[michalkleiner]

The business logic still needs to be resolved (whether we do fixed values, top and bottom percentage or something else). There's also a merge conflict now which needs to be resolved, plus a change request from Stefan.

I've removed the Needs review tag for now until all the above is addressed at which point it can be marked as Needs review again.

[rr-it]

On the business logic - most points were already mentioned:

• Clear preference to cap by top percentage over fixed values:
  • But: Top percentage is more implementation work.
  • Fixed values already archive the target to make the chart meaningful. (Right now the charts informational value is insignificant.)
  • There is already a fixed value cap. The Maximum Value for Unsigned MEDIUMINT of database: 16777215 ms ~ 4 h 39 min 37 s
• Top and bottom? No need for bottom value caps as the impact of low values is neglectable.

Without this PR

No outliers

• 10000 values of 250 ms result in an average of 0.250 seconds
  $\frac{10000×250 ms}{10000} = 250 ms$

Top outlier

• One value of 4 h and 10000 values of 250 ms result in an average of ~ 1.690 seconds
  • $4h = 4×60×60×1000 ms = 14400000 ms$
  • $\frac{14400000 ms + 10000×250 ms}{10001} = 1690 ms$

Bottom outlier

• One value of 1 ms and 10000 values of 250 ms result in an average of ~ 0.250 seconds
  • $\frac{1 ms + 10000×250 ms}{10001} = 249.975 ms$

[michalkleiner]

@Stan-vw I guess it's your call on whether we should accept this as is or you want any adjustments.

[Stan-vw]

@michalkleiner it's been ages since we discussed this so I could be wrong, but I believe we agreed to merge this without front end configuration option for now. I'm actually surprised to see it's still open rather than merged, what part is not mergable yet?

[rr-it]

@sgiehl You merged branch '5.x-dev' into issue/17847-performance-report-cap-values.

If you like I can revert this merge and instead rebase my changes onto branch '5.x-dev'. And then do a force push of this newly created branch for issue/17847-performance-report-cap-values.

Done: I proceeded as described above.

[rr-it]

The following code throws WARNING/NOTICE in "Matomo Tests / UI (0-3)":

https://github.com/rr-it/matomo/blob/27e6b666b01eea12acefec6271622ed90716ddc9/plugins/PagePerformance/Columns/Base.php#L29-L34

        try {
            $valueCap = Config::getInstance()->PagePerformance[$this->columnName . '_cap_' . $this->type];
        } catch (Exception $ex) {
            // 0 disables cap
            return 0;
        }

The WARNING/NOTICE is like:

2024-01-03T15:31:12.7143577Z WARNING PagePerformance
[2024-01-03 15:24:08 UTC] [5529] /home/runner/work/matomo/matomo/matomo/plugins/PagePerformance/Columns/Base.php(30):
Notice - Undefined index: time_network_cap_duration_ms - Matomo 5.0.0 - Please report this message in the Matomo forums: https://forum.matomo.org (please do a search first as it might have been reported already)
#0/plugins/PagePerformance/Columns/Base.php(30),
#1/plugins/PagePerformance/Columns/Base.php(40),
#2/plugins/PagePerformance/Metrics.php(115),
#3/plugins/PagePerformance/PagePerformance.php(151),
[internal function]: Piwik\Plugins\PagePerformance\PagePerformance->addActionMetrics(),
#5/core/EventDispatcher.php(147),
#6/core/Piwik.php(870),
#7/plugins/Actions/Metrics.php(89),
#8/plugins/Actions/RecordBuilders/ActionReports.php(194),
#9/plugins/Actions/RecordBuilders/ActionReports.php(81)

How do you correctly call Config::getInstance()->PagePerformance['this_index_might_be_not_set']; without getting a WARNING/NOTICE?

[rr-it]

On runs of "Matomo Tests / UI (0-3)" the php-log does not become cluttered anymore with PHP NOTICE-entries.

The added ?? 0 does the trick to hide NOTICE: undefined index. And it also ensures that on missing configuration the default becomes 0:
$valueCap = Config::getInstance()->PagePerformance['this_index_might_be_not_set'] ?? 0;

@sgiehl Now I think this PR is ready to merge.

[github-actions]

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

[sgiehl]

Sorry for not coming back earlier on this one. I would suggest to update the configuration slightly. Otherwise this imho should look good. We could though consider to add some tests around that, to ensure it works correctly.

[sgiehl]

I've added a simple integration test now to cover the value capping in a test. Imho this should be good to merge now. We might need to update the config ui test after merging though

[michalkleiner]

@sgiehl should this be called testShouldCapOutlinerValuesWhenConfigured() without the 'not'?

[sgiehl]

indeed