Skip to content

Commit

Permalink
Fix pagination in getAllCountsByColumn (#123)
Browse files Browse the repository at this point in the history 
* getAllCounts needs two separate queries

* Fix test

* Fix remaining tests
  • Loading branch information
@benvinegar
benvinegar authored Dec 12, 2024
1 parent db24a2b commit da2aecb
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 41 deletions.
32 changes: 19 additions & 13 deletions app/analytics/__tests__/query.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,18 +295,31 @@ describe("AnalyticsEngineAPI", () => {

describe("getAllCountsByColumn", () => {
test("it should return an array of [column, count] tuples", async () => {
fetch.mockResolvedValue(
// return 2 mocked responses
fetch.mockResolvedValueOnce(
createFetchResponse({
data: [
{
blob4: "CA",
isVisitor: 1,
count: 3,
},
],
}),
);
fetch.mockResolvedValueOnce(
createFetchResponse({
data: [
{
blob4: "CA",
isVisitor: 0,
count: 1,
},
],
}),
);

const result = api.getAllCountsByColumn(
const result = await api.getAllCountsByColumn(
"example.com",
"country",
"7d",
Expand All @@ -316,24 +329,17 @@ describe("AnalyticsEngineAPI", () => {
},
);

expect(fetch).toHaveBeenCalled();
expect(fetch).toHaveBeenCalledTimes(2);
expect(
(fetch as Mock).mock.calls[0][1].body
.replace(/\s+/g, " ") // removes tabs and whitespace from query
.trim(),
).toEqual(
"SELECT blob4, " +
"double1 as isVisitor, " +
"double3 as isBounce, " +
"SUM(_sample_interval) as count " +
"FROM metricsDataset WHERE timestamp >= NOW() - INTERVAL '7' DAY AND timestamp < NOW() AND blob8 = 'example.com' AND blob4 = 'CA' " +
"GROUP BY blob4, double1, double3 " +
"ORDER BY count DESC LIMIT 10",
);
// console.log(result);
expect(await result).toEqual({
CA: {
views: 3,
visitors: 0,
views: 4,
visitors: 3,
bounces: 0,
},
});
Expand Down
40 changes: 34 additions & 6 deletions app/analytics/query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,26 +430,43 @@ export class AnalyticsEngineAPI {
tz,
);

const filterStr = filtersToSql(filters);
// first query by visitor count – this is to figure out the top N results
// by visitor count first
// NOTE: there's an await here; need to fix this or harms parallelism
const visitorCountByColumn = await this.getVisitorCountByColumn(
siteId,
column,
interval,
tz,
filters,
page,
limit,
);

// next, make a second query - this time for non-visitor hits - by filtering
// on the keys returned by the first query.
const keys = visitorCountByColumn.map(([key]) => key);

const filterStr = filtersToSql(filters);
const _column = ColumnMappings[column];
const query = `
SELECT ${_column},
${ColumnMappings.newVisitor} as isVisitor,
${ColumnMappings.bounce} as isBounce,
SUM(_sample_interval) as count
FROM metricsDataset
WHERE timestamp >= ${startIntervalSql} AND timestamp < ${endIntervalSql}
AND ${_column} IN (${keys.map((key) => `'${key}'`).join(", ")})
AND ${ColumnMappings.newVisitor} = 0
AND ${ColumnMappings.siteId} = '${siteId}'
${filterStr}
GROUP BY ${_column}, ${ColumnMappings.newVisitor}, ${ColumnMappings.bounce}
GROUP BY ${_column}, ${ColumnMappings.newVisitor}
ORDER BY count DESC
LIMIT ${limit * page}`;

type SelectionSet = {
readonly count: number;
readonly isVisitor: number;
readonly isBounce: number;
count: number;
isVisitor: number;
isBounce: number;
} & Record<
(typeof ColumnMappings)[T],
ColumnMappingToType<(typeof ColumnMappings)[T]>
Expand All @@ -475,6 +492,16 @@ export class AnalyticsEngineAPI {
limit * page,
);

// remap visitor counts into SelectionSet objects, then insert into
// the query results (pageData)
visitorCountByColumn.forEach(([key, value]) => {
pageData.push({
[_column]: key,
count: value,
isVisitor: 1,
} as SelectionSet);
});

const result = pageData.reduce(
(acc, row) => {
const key = row[_column] as string;
Expand All @@ -491,6 +518,7 @@ export class AnalyticsEngineAPI {
},
{} as Record<string, AnalyticsCountResult>,
);

resolve(result);
})(),
);
Expand Down
29 changes: 15 additions & 14 deletions app/routes/__tests__/resources.paths.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,30 +25,29 @@ describe("Resources/Paths route", () => {
});
describe("loader", () => {
test("returns valid json", async () => {
// first request for visitors
fetch.mockResolvedValueOnce(
createFetchResponse({
data: [
{ blob3: "/", isVisitor: 0, isVisit: 0, count: "5" },
{ blob3: "/", isVisitor: 0, isVisit: 1, count: "1" },
{ blob3: "/", isVisitor: 1, isVisit: 1, count: "2" },
{ blob3: "/", isVisitor: 1, count: "2" },
{
blob3: "/example",
isVisitor: 0,
isVisit: 0,
isVisitor: 1,
count: "4",
},
],
}),
);
// second request for views
fetch.mockResolvedValueOnce(
createFetchResponse({
data: [
{ blob3: "/", isVisitor: 0, count: "5" },
{
blob3: "/example",
isVisitor: 0,
isVisit: 1,
count: "6",
},
{
blob3: "/example",
isVisitor: 1,
isVisit: 1,
count: "2",
},
],
}),
);
Expand All @@ -63,12 +62,14 @@ describe("Resources/Paths route", () => {

// expect redirect
expect(response.status).toBe(200);
expect(fetch).toHaveBeenCalledTimes(2);

const json = await response.json();

expect(json).toEqual({
countsByProperty: [
["/", 2, 8],
["/example", 2, 12],
["/example", 4, 10],
["/", 2, 7],
],
page: 1,
});
Expand Down
23 changes: 15 additions & 8 deletions app/routes/__tests__/resources.referrer.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,28 @@ describe("Resources/Referrer route", () => {

describe("loader", () => {
test("returns valid json", async () => {
// first request for visitors
fetch.mockResolvedValueOnce(
createFetchResponse({
data: [
{ blob5: "/", isVisitor: 0, isVisit: 0, count: "5" },
{ blob5: "/", isVisitor: 1, isVisit: 1, count: "1" },
{ blob5: "/", isVisitor: 1, count: "1" },
{
blob5: "/example",
isVisitor: 0,
isVisit: 1,
count: "2",
isVisitor: 1,
count: "1",
},
],
}),
);
// second request for views
fetch.mockResolvedValueOnce(
createFetchResponse({
data: [
{ blob5: "/", isVisitor: 0, count: "5" },
{
blob5: "/example",
isVisitor: 1,
isVisit: 1,
count: "1",
isVisitor: 0,
count: "2",
},
],
}),
Expand All @@ -57,6 +63,7 @@ describe("Resources/Referrer route", () => {

// expect redirect
expect(response.status).toBe(200);
expect(fetch).toHaveBeenCalledTimes(2);

const json = await response.json();
expect(json).toEqual({
Expand Down

0 comments on commit da2aecb

Please sign in to comment.