Integrate archiveReports with ArchiveMetrics timer #23906
Conversation
caddoo
force-pushed
the
dev-19750-timing-metrics
branch
from
7d18ad1 to
dd9f1d2
Compare
caddoo
force-pushed
the
dev-19750-timing-metrics
branch
from
887b7ba to
f08ed6e
Compare
| $segmentObj | ||
| ); | ||
| if ($report) { | ||
| $parameters->setArchiveOnlyReport($report); |
There was a problem hiding this comment.
Is it relevant for the current metric gathering if a single report was requested? For example, when a new custom report is being archived?
There was a problem hiding this comment.
I've decided now to skip for individual reports, i've left the condition at this level, is that going to be confusing do you think, that there is a decision here and internally in the timer.
There was a problem hiding this comment.
I did not mean to add any skip-check here, I would actually not. The event handler should take care of not doing work on anything it receives but does not want.
Phrasing my question differently:
Would it make sense to add $report to the event signature? It is part of the
archiveReportssignature, and dropping it feels like a rather arbitrary decision.
There was a problem hiding this comment.
This is now used and removed $shouldRecordMetrics
plugins/CoreAdminHome/tests/Integration/ArchiveReportsMetricsTimerTest.php
Outdated
Show resolved
Hide resolved
plugins/CoreAdminHome/tests/Integration/ArchiveReportsMetricsTimerTest.php
Outdated
Show resolved
Hide resolved
plugins/CoreAdminHome/tests/Integration/ArchiveReportsMetricsTimerTest.php
Outdated
Show resolved
Hide resolved
|
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
mneudert
left a comment
mneudert
left a comment
There was a problem hiding this comment.
Merging into main PR. Will add any comments (e.g. for the broken CI parts) there.
* Integrate archiveReports with ArchiveMetrics timer * Create event and hook timer into it * Add better typing * Make sure plugin is loaded for integration test * Skip metrics for single-report archiving * Add test-only timer reset helper * Simplify archive loader test GET handling * Add nullable type for instance * cs fix * Add strict types * Allow reports to have metrics recorded
* Integrate archiveReports with ArchiveMetrics timer * Create event and hook timer into it * Add better typing * Make sure plugin is loaded for integration test * Skip metrics for single-report archiving * Add test-only timer reset helper * Simplify archive loader test GET handling * Add nullable type for instance * cs fix * Add strict types * Allow reports to have metrics recorded
Description
Integrates with this work #23902
Not every path is tested as the timer is tested in the other work.
Checklist
Review