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

Output Segment ID or Name with Exception #18436

Open
Starker3 opened this issue Dec 2, 2021 · 6 comments
Open

Output Segment ID or Name with Exception #18436

Starker3 opened this issue Dec 2, 2021 · 6 comments
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Easier debugging For issues that make troubleshooting issues for developers easier. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Milestone
Backlog (Help wan...

Comments

@Starker3
Copy link
Contributor

Starker3 commented Dec 2, 2021

When users run a database update and have segments that use unsupported segment conditions (For example when a plugin was removed or a dimension disabled) it results in an exception during the database upgrade.

For example, when a dimension is disabled but used in a segment we will see the following sort of error:

./console core:update --yes -vvv
*** Update ***
Database Upgrade Required
Your Matomo database is out-of-date, and must be upgraded before you can continue.
Matomo database will be upgraded from version 4.5.0 to the new version 4.6.1.

ERROR [2021-12-01 11:47:55] 51704 Uncaught exception: /matomo/core/Segment.php(215): Segment 'dimension1' is not a supported segment. [Query: , CLI mode: 1]
DEBUG [2021-12-01 11:47:55] 51704 Loaded plugins: (...)

[Exception]
Segment 'dimension1' is not a supported segment.

Exception trace:
(...)

It would be much easier to troubleshoot if we were to output the Segment name or Segment ID that has the unsupported condition.
@Starker3 Starker3 added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Dec 2, 2021
@tsteur tsteur added this to the Priority Backlog (Help wanted) milestone Dec 2, 2021
@tsteur
Copy link
Member

tsteur commented Dec 2, 2021

refs #16152

As we don't usually no the segmentId there easily, we could additionally mention the idSite at least at some point and the full segment definition if possible.

Technically could also do DB queries to find out the idSegment under circumstances, but depending on the encoding and other things this might not always work etc.

@tsteur tsteur added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Dec 2, 2021
@sgiehl
Copy link
Member

sgiehl commented Dec 3, 2021

@tsteur I guess this might most likely also be a regression we should handle in 4.6.2. The problem is the update script, which tries to iterate over all segments here:

matomo/core/Updates/4.6.0-b4.php

Lines 51 to 77 in 4fe950a

foreach ($idSites as $idSite) {
$segmentStrings = Rules::getSegmentsToProcess([$idSite]);
foreach ($segmentStrings as $segmentString) {
$segment = new Segment($segmentString, [$idSite]);
if ($segment->getOriginalString() === $segment->getString()) {
continue;
}
$segmentsToAppend = [VisitFrequencyAPI::NEW_VISITOR_SEGMENT, VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT];
foreach ($segmentsToAppend as $segmentToAppend) {
// we need to migrate the existing archive
$oldSegmentString = Segment::combine($segment->getString(), SegmentExpression::AND_DELIMITER, $segmentToAppend);
$newSegmentString = Segment::combine($segment->getOriginalString(), SegmentExpression::AND_DELIMITER, $segmentToAppend);
$oldSegmentHash = Segment::getSegmentHash($oldSegmentString);
$newSegmentHash = Segment::getSegmentHash($newSegmentString);
if ($oldSegmentHash === $newSegmentHash) {
continue;
}
$doneFlagsToMigrate['done' . $oldSegmentHash . '.Goals'] = 'done' . $newSegmentHash . '.Goals';
$doneFlagsToMigrate['done' . $oldSegmentHash . '.VisitsSummary'] = 'done' . $newSegmentHash . '.VisitsSummary';
$doneFlagsToMigrate['done' . $oldSegmentHash . '.UserCountry'] = 'done' . $newSegmentHash . '.UserCountry';
}
}
}

I guess we should do a try/catch around the segment construct and simply skip segments that cause an exception

@tsteur tsteur mentioned this issue Dec 5, 2021
Closed
@tsteur
Copy link
Member

tsteur commented Dec 5, 2021

@sgiehl 👍 created #18451

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2021

@tsteur can't we actually close this issue now? The update problem should be solved. And all other issues regarding invalid segments should be covered with #16152

@tsteur
Copy link
Member

tsteur commented Dec 7, 2021

@sgieh it depends how things are solved. If the segment class itself still throughs an exception, and it's eg used in an update script or in a command or anywhere else where it's not caught or handled etc, then it would be still useful to show more information what segment this comes from.

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2021

ah. ok. that makes sense

@mattab mattab added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Easier debugging For issues that make troubleshooting issues for developers easier. labels Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Easier debugging For issues that make troubleshooting issues for developers easier. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Milestone
Backlog (Help wanted)
Development

No branches or pull requests
4 participants
@tsteur @mattab @sgiehl @Starker3