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
Faster segmented suggested values when browser archiving is disabled #15786
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Otherwise looks good and works 👍
Should we maybe create a follow up issue to add that feature to other dimensions as well? 🤔
public function setSuggestedValuesApi($suggestedValuesApi) | ||
{ | ||
if (!empty($suggestedValuesApi) && is_string($suggestedValuesApi)) { | ||
if (Development::isEnabled() && strpos($suggestedValuesApi, '.get') === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess we can ignore that this might throw a warning in development mode if setSuggestedValuesApi
is called with something else than a string
plugins/API/API.php
Outdated
} | ||
} | ||
|
||
$flat = strpos($segment['suggestedValuesApi'], 'Actions.') === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if there would be a better solution than hardcoding that for Actions
only. Maybe we could always flatten the report if the API method has subtables with the same dimension or something like that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it hard coded flattened initially, but then noticed there was an issue with the content dimensions for example where it then would return content name - content piece
instead of just content name
. technically, I suppose we could check the metadata and see if there is a subtableApiMethod defined and whether it is the same API method or a different one. Should there be a different api method we could disable flattening. Was hoping though to not needing to do it simply to keep things simple. I suppose it wouldn't be that complex though maybe. Will check later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it @sgiehl
plugins/API/API.php
Outdated
@@ -624,8 +626,6 @@ public function getSuggestedValuesForSegment($segmentName, $idSite) | |||
); | |||
if (!empty($segment) && preg_match('/^' . implode('|',$remove) . '/', $segment)) { | |||
$values[] = urldecode(urldecode(str_replace($remove, '', $segment))); | |||
} else { | |||
$values[] = $row->getColumn('label'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing that actually requires a segment metadata to be set, right? should we maybe mention that in the doc block of setSuggestedValuesApi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
* Avoid possible error subtable already exists but not loaded (#15779) * Make sure to always set JSON_PIWIK to native JSON when possible (#15785) * make sure to always set JSON_PIWIK to native JSON when possible * rebuilt piwik.js * Force POST for bulk requests, fix alwaysUseSendBeacon not respected for bulk requests (#15784) * Force POST for bulk requests, fix alwaysUseSendBeacon not respected for bulk requests * rebuilt piwik.js * Make sure to clean up tracking failures before sending email notification (#15798) Feedback from a customer... Eg the daily `cleanupTrackingFailures()` action might be only executed after the weekly `notifyTrackingFailures` therefore we should try to clean up failures first and then check if any are left. Avoids the case where a user opens hours later the email they receive and then there are no tracking failures reported. This could still happen but it's a bit less likely. * 3.13.5-b1 * Faster segmented suggested values when browser archiving is disabled (#15786) * Faster segmented suggested values when browser archiving is disabled * make sure no segment is set * remove wrong var type * fix/add tests * add more segment values * detect if we should flatten or not * add docs * Fix problem when comparing segments or opening popovers (#15809) refs #15805 * purge all old archives regardless of done value (#15800) * purge all old archives regardless of done value, we only care about the newest usable one * Fix test and start on new one. * Add coverage for change in tests. * there is no longer an inner join so should not need the idsite check * Add more parameters to the computeNbUnique event (#15808) * 3.13.5-b2 * One click update in two parts so new code is loaded for second. (#15770) * One click update in two parts so new code is loaded for second. * remove no longer needed code Co-authored-by: Thomas Steur <tsteur@users.noreply.github.com> Co-authored-by: Matthieu Aubry <mattab@users.noreply.github.com> Co-authored-by: Stefan Giehl <stefan@matomo.org>
…atomo-org#15786) * Faster segmented suggested values when browser archiving is disabled * make sure no segment is set * remove wrong var type * fix/add tests * add more segment values * detect if we should flatten or not * add docs
…atomo-org#15786) * Faster segmented suggested values when browser archiving is disabled * make sure no segment is set * remove wrong var type * fix/add tests * add more segment values * detect if we should flatten or not * add docs
fix #15732
Also fixes an issue where the wrong segment metadata was created for Page URL/Title not defined and eg segmented visitor log did not work for these.