Premium Analytics: add Stats traffic queries - #49777
Conversation
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
|
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
e10f0b5 to
0d72822
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.
734f111 to
b192b40
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.
|
Review loop clear for current head |
e9be8ec to
5d078f0
Compare
| ], | ||
| queryFn: async () => { | ||
| const response = await fetchStatsProxy( { version, endpoint, params, method, body } ); | ||
| return statsSanitizers[ sanitizer ]( response, params ) as TData; |
There was a problem hiding this comment.
Why we need to use as TData here? Is it possible to derive from the sanitizer?
There was a problem hiding this comment.
Addressed in 3e0a3196cd. I removed the free TData generic and the as TData cast. statsProxyQuery now returns the sanitizer-map return union, so callers cannot provide an arbitrary result type that disagrees with the selected sanitizer.
There was a problem hiding this comment.
We're using reportOnlyKeys as a denylist to delete any invalid params. If there is any changes to ReportParams, it silently leaks into every Stats request.
The existing Woo fetch helpers do the opposite, it explicitly pick the fields they send. Is it possible to use same approach here to ensure we only allow specific params?
There was a problem hiding this comment.
Addressed before the latest push and still in place: reportParamsToStatsQueryParams now explicitly picks the allowed Stats params instead of deleting a denylist of Woo/report-only params. I also kept a test covering unknown/report-only params so future ReportParams additions do not silently leak into report Stats requests.
| @@ -0,0 +1,152 @@ | |||
| /** | |||
| * External dependencies | |||
| */ | |||
There was a problem hiding this comment.
Let's remove this redundant comment. 🙂
There was a problem hiding this comment.
Removed in 3e0a3196cd.
There was a problem hiding this comment.
Any reason that we need to create a new type that alias to StatsProxyParams? Looks like StatsProxyParams is already a type and exported,.
There was a problem hiding this comment.
Addressed earlier: the extra alias was removed and callers use the exported StatsProxyParams type directly.
| days?: number; | ||
| num?: number; | ||
| max?: number; | ||
| summarize?: number | boolean; | ||
| [ key: string ]: unknown; |
There was a problem hiding this comment.
nit: Can we make StatsQueryParamInput extend from StatsQueryParams? It repeats the same 7-field.
There was a problem hiding this comment.
Partially addressed: the repeated explicit field list is gone and the output still uses the explicit StatsQueryParamFields allowlist. I kept the input type flexible because raw ReportParams can include non-proxy values such as filters objects; narrowing the input to StatsQueryParams makes those allowlist tests fail at compile time instead of proving they are dropped.
There was a problem hiding this comment.
Should a missing requested date return empty instead of an arbitrary day? or would falling back to the sole bucket only when days has exactly one entry (empty otherwise) be safer?
There was a problem hiding this comment.
I did not change this one in #49777. This lives in the normalizer utility code from the already-merged #49776 rather than the new query-factory layer. For this PR I fixed the query-side range ambiguity by sending summarize: 1 for multi-day report ranges; if we still want to tighten missing-date fallback behavior in getStatsBuckets, I would do that as a small follow-up against trunk.
|
|
||
| function datePart( value?: string ) { | ||
| return value?.split( 'T' )[ 0 ]; | ||
| } | ||
|
|
||
| function daysBetweenInclusive( from: string, to: string ) { |
There was a problem hiding this comment.
Similar to another PR, can we use date-fns or @jetpack-premium-analytics/datetime methods, instead of creating helpers here?
For example, we can probably use differenceInCalendarDays to replace this method.
There was a problem hiding this comment.
Addressed earlier by using the shared getDaysBetweenInclusive helper from utils/interval, which wraps date-fns differenceInCalendarDays with the inclusive +1 behavior and invalid-range guard we need here.
| @@ -79,7 +93,7 @@ export function reportParamsToStatsQueryParams( | |||
| const date = params.date ?? to; | |||
| const startDate = params.start_date ?? from; | |||
| const days = | |||
| params.days ?? ( startDate && date ? getDaysBetweenInclusive( startDate, date ) : undefined ); | |||
| params.days ?? ( startDate && date ? daysBetweenInclusive( startDate, date ) : undefined ); | |||
There was a problem hiding this comment.
This computes days for the full selected range and and statsReportQuery sends it to the proxy, but it never sets summarize. In getStatsBucket, it then falls through to days[date], ends up showing one day's totals instead of the whole range.
Could statsReportQuery pass summarize: 1 when the range spans more than one day? Or if single-day is the intended behavior
There was a problem hiding this comment.
Addressed in 3e0a3196cd: statsReportQuery now adds summarize: 1 for multi-day ranges when the caller did not explicitly provide summarize. I added tests for the default multi-day summarized request and for preserving an explicit summarize: false.
There was a problem hiding this comment.
There was a problem hiding this comment.
While reviewing #49568 (the Top posts widget port) I noticed it resolves the day bucket differently and I think more correctly. If I understand correctly, WPCOM keys the days bucket by the period start date, not the requested reference date. Could this layer adopt the same approach?
There was a problem hiding this comment.
Good catch. I did not change this in #49777 because it is also in the normalizer utility layer from #49776, not in the query factory diff. The current #49777 fix avoids the multi-day aggregate ambiguity by requesting summarized data for multi-day ranges. I agree the period-start bucket resolution should be handled in a focused follow-up against trunk so week/month raw bucket lookup matches the Top Posts port.
There was a problem hiding this comment.
53177fe to
f4fdbf2
Compare
b192b40 to
3bf4050
Compare
3bf4050 to
9d1cf8b
Compare
|
@claude review plz |
|
Claude finished @kangzj's task in 1m 50s —— View job Claude PR Review
Nice, clean split — the per-endpoint query files are tidy and the shared 1.
|
|
@claude review plz |
|
Claude finished @kangzj's task in 1m 42s —— View job Claude PR Review (re-review at
|
| ...statsParams, | ||
| ...( statsParams.summarize === undefined && | ||
| typeof statsParams.days === 'number' && | ||
| statsParams.days > 1 |
There was a problem hiding this comment.
statsReportQuery is a report-specific wrapper around statsProxyQuery.
It accepts Woo/report-style date params like from, to, interval, converts them into Stats params like period, start_date, end_date/API date,
computes days, enables the query only when a date range exists, and now defaults multi-day ranges to summarize: 1.
Fixes #
Proposed changes
datemapping.end_dateto the APIdateparam, and keeping query files per endpoint instead of a mixed query module.Related product discussion/links
trunk.Does this pull request change what data or activity we track or use?
No. This adds client query wrappers for existing Stats API/proxy endpoints.
Testing instructions
pnpm --dir projects/packages/premium-analytics test --runInBand.pnpm --dir projects/packages/premium-analytics typecheck.pnpm --dir projects/packages/premium-analytics build.jetpack.com, inspect dashboard/detail/period-shortcut requests, and confirm summarized requests still sendsummarize=1. Replay equivalent authenticated requests withoutsummarize=1and confirm raw responses expose top-leveldate,period,days, endpoint-specific bucket arrays, and scalar totals where present.