Premium Analytics: add Stats traffic normalizers#49776
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
35c63e7 to
da463c7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Review loop clear for current head |
louwie17
left a comment
There was a problem hiding this comment.
I left a couple thoughts, a bit around simplification. Also that we may want to change the structure of the normalized data a bit to help simplify the normalization for each endpoint.
For example each item relies on views and a label most of the other things could be considered meta.
| const items = query?.summarize | ||
| ? asArray< AnyRecord >( asRecord( response ).summary?.postviews ) | ||
| : asArray< AnyRecord >( bucket.postviews ); |
There was a problem hiding this comment.
This seems a bit redundant given getStatsBucket, as it basically does the same thing as far as I am understanding.
This is duplicated in each function, so maybe we can clean this up. Also asRecord sounds a bit confusing. Do we need this? Can't we use response?.<key> in that case?
| summary: numericSummary( { | ||
| total: | ||
| bucket.total ?? | ||
| groups.reduce( ( total, item ) => total + safeParseFloat( item.total ?? item.views ), 0 ), |
There was a problem hiding this comment.
We are doing the reduce in a lot of places as well to count the total amount. Maybe a function countTotalByKey or something would help make this a bit more readable.
Also what does numericSummary exactly do? it seems like it enforces the summary to be a float? Maybe we can highlight that in the function call as well.
Also curious in what scenarios we run into either a string or number that it has to be explicitly converted. There seem to be a lot of handling of potential edge cases, but not sure if they are real?
| actions?: StatsItemAction[]; | ||
| actionMenu?: number; | ||
| [ key: string ]: unknown; | ||
| }; |
There was a problem hiding this comment.
It may be helpful to split this up a bit into data that is shown in a chart ( labels, values ) and things that may be required for adding custom labels or additional data points, that could be considered meta data that is more specific to the widget it-self.
This may help simplify the parsing below as well.
This is just a thought, I did not double check if that seems reasonable.
Fixes #
Proposed changes
What changed and why
This is PR 2 in the split from #49773, stacked on #49775. It keeps the data-shaping layer separate from the query/hook API so reviewers can validate the normalized report contract before endpoint wiring is added.
The normalizers convert Jetpack Stats proxy responses into the existing Premium Analytics-style report shape: a numeric
summaryplusdatarows with shared keys likelabel,value,children,link, and action metadata. That keeps Stats widgets and dashboards from needing endpoint-specific response handling.Non-test/support implementation size: 344 lines including changelog and exports.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
pnpm --dir projects/packages/premium-analytics test.pnpm --dir projects/packages/premium-analytics typecheck.pnpm --dir projects/packages/premium-analytics build.