Skip to content
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

Jump to bottom

Add date period to segment archive queries #17330

Closed
tsteur opened this issue Mar 10, 2021 · 1 comment · Fixed by #17337
Closed

Add date period to segment archive queries #17330

tsteur opened this issue Mar 10, 2021 · 1 comment · Fixed by #17337
Assignees
@flamisz
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Milestone
4.3.0

Comments

@tsteur
Copy link
Member

tsteur commented Mar 10, 2021 •
edited

When there is an archiving query for a segment, then we don't know which time period this query is selecting because the time period was already included in the query when the temporary table was created (https://github.com/matomo-org/matomo/blob/4.2.1/core/DataAccess/LogAggregator.php#L273)

image

When a segment is used, then we'd basically need to add another comment here including the date: https://github.com/matomo-org/matomo/blob/4.2.1/core/DataAccess/LogAggregator.php#L400

Eg if it queries the day for 2020-02-02 then there be a comment /* 2020-02-02 */ . We wouldn't need to know the exact time range used but could do that if it's easier. We could also generally add this comment and adjust the query hint accordingly around https://github.com/matomo-org/matomo/blob/4.2.1/core/ArchiveProcessor/PluginsArchiver.php#L153

If it's easily possible, then we'd also want to add the segmentId (might be already the case)

While working on this could then maybe also quickly look at #17327

Technically this should be already the case but maybe this is not working? https://github.com/matomo-org/matomo/blob/4.2.1/core/DataAccess/LogAggregator.php#L406-L408

We would expect to see something like SELECT /* 2020-02-02,2020-02-03 sites 1,2,3 */ /* plugin name /* ...

This should only apply to archiver queries when you for example execute ./console core:archive and it is executing a query of some plugin archiver.
@tsteur tsteur added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Mar 10, 2021
@tsteur tsteur added this to the 4.3.0 milestone Mar 10, 2021
@flamisz flamisz self-assigned this Mar 10, 2021
@flamisz
Copy link
Contributor

flamisz commented Mar 11, 2021

I'm investigation this issue and I see something that can be the root of the issue (or at least that's why a couple of things should work but not working).

In this part of the code:

matomo/core/DataAccess/LogAggregator.php

Lines 400 to 411 in 3688fff

$select = 'SELECT';
if ($this->queryOriginHint && is_array($query) && 0 === strpos(trim($query['sql']), $select)) {
$query['sql'] = trim($query['sql']);
$query['sql'] = 'SELECT /* ' . $this->queryOriginHint . ' */' . substr($query['sql'], strlen($select));
}
if (!$this->getSegment()->isEmpty() && is_array($query) && 0 === strpos(trim($query['sql']), $select)) {
$query['sql'] = trim($query['sql']);
$query['sql'] = 'SELECT /* ' . $this->dateStart->toString() . ',' . $this->dateEnd->toString() . ' sites ' . implode(',', array_map('intval', $this->sites)) . ' segmenthash ' . $this->getSegment()->getHash(). ' */' . substr($query['sql'], strlen($select));
}
return $query;

we basically check if the the query starts with SELECT, but when we use the SegmentQueryDecorator here:

matomo/plugins/SegmentEditor/SegmentQueryDecorator.php

Lines 44 to 46 in 3688fff

if (SettingsServer::isArchivePhpTriggered()) {
$prefixParts[] = 'trigger = CronArchive';
}

we prefix the SELECT query with /* trigger = CronArchive */ string (and we add new line as well), so we don't add the queryOriginHint and dates and other useful information for the query as comment in the LogAggregator class.

The first time I use this feature, so maybe I'm wrong, but this is the result of my first investigation of this issue.
Do you think it is the problem? Ping @tsteur @sgiehl

@flamisz flamisz mentioned this issue Mar 11, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@flamisz flamisz

Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging a pull request may close this issue.

modify SegmentQueryDecorator so extra comment information can be saved matomo-org/matomo
2 participants
@tsteur @flamisz