Add nb_profilable metric counting profilable visits. #19745
Conversation
|
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
.../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.
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.
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.
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.
I'll see if I can make it behave as before.
sgiehl
removed
Needs Review
|
@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 |
github-actions
bot
added
the
Stale
|
@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 |
github-actions
bot
removed
the
Stale
|
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
|
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 |
github-actions
bot
added
the
Stale for long
|
@justinvelluppillai Has there been a decision whether to merge this into the next 4.x patch release or keep it for 5.x? |
github-actions
bot
removed
Stale
|
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
|
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 |
github-actions
bot
added
the
Stale for long
|
@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. |
sgiehl
removed
Needs Review
|
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
github-actions
bot
added
the
Stale
mattab
added
the
Do not close
github-actions
bot
removed
the
Stale
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