-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add nb_profilable metric counting profilable visits. #19745
base: 4.x-dev
Are you sure you want to change the base?
Conversation
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
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 a couple of comments. Besides that I guess the PR needs to be rebased to 5.x-dev.
.../PHPUnit/System/expected/test_OneVisitorTwoVisits_hideAllColumns___VisitsSummary.get_day.xml
Outdated
Show resolved
Hide resolved
<nb_uniq_visitors_returning>0</nb_uniq_visitors_returning> | ||
<nb_users_returning>0</nb_users_returning> | ||
<nb_visits_returning>0</nb_visits_returning> | ||
<nb_actions_returning>0</nb_actions_returning> | ||
<nb_visits_converted_returning>0</nb_visits_converted_returning> | ||
<bounce_count_returning>0</bounce_count_returning> |
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.
Are you able to explain why there are so many test file updates, where a lot other metrics (than only the profilable metric) has been added?
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 haven't verified, but I'm guessing the returning VisitsSummary.get API method returned an empty DataTable (w/ no metrics instead of 0 values). Something must have changed here to return a DataTable w/ entries for 0 metric values. Is it important?
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 guess the reason might be, that for nb_profilable
there is a special handling in the archive writer to also write empty archives. As the additional rows are result of an segment, I guess the segment doesn't have any visits and before no archives were written and such no values were available. Now the nb_profilable archive is written nevertheless and the api requests for that segment seems to return 0 values instead.
I think for the VisitFrequency result it's better to have 0 values, than not having the metrics included at all. But not sure if this might have other implications maybe.
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'll see if I can make it behave as before.
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
@sgiehl modified the code and asked questions for other items |
@mattab @justinvelluppillai any update on ^? |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
@mattab I am happy either way, you can maybe decide here? It is likely there will be a 4.13 to release a few things that need to be done for this quarter |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers |
@justinvelluppillai Has there been a decision whether to merge this into the next 4.x patch release or keep it for 5.x? |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers |
@diosmosis What's the state of this PR? It will for sure not got to 4.x anymore, as we are already preparing to release Matomo 5. |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
Description:
Refs #16363
I'm finishing up #16773, and thought it would be better to review/merge it piecemeal. This PR adds nb_profilable as a metric to VisitsSummary.get. It's just the sum of all profilable visits, and will be used in further PRs.
Review