APIv2: Replace breakdown module with QueryBuilder #4283

macobo · 2024-06-27T07:22:27Z

Changes

This PR starts using the new QueryBuilder in breakdown module with a compatibility layer for the old response schema.

The following changes also happen as a side-effect:

bounce_rate is reported as 0 not nil when site is not entry page
revenue metrics are handled in breakdown module. When added to apiv2 it would be wise to include currency in response and add validation errors when site does not have access to revenue
time_on_page is a special case within the breakdown module.
imports.ex gets significantly changed (to remove old compatibility hacks)
QueryOptimizer also changes event:page to visit:entry_page filter when doing a breakdown by event:page and querying session metrics. Required for imports.
Garbage filters (which pass validation) cause query to return false
Breakdown module default ordering subtly changes, causing changes in some tests.
Simplify query building around order_by

Depends on #4251

lib/plausible/stats/breakdown.ex

ukutaht · 2024-06-27T09:07:22Z

lib/plausible/stats/breakdown.ex

+  defp result_key("event:props:" <> custom_property), do: custom_property
+  defp result_key("event:" <> key), do: key |> String.to_atom()
+  defp result_key("visit:" <> key), do: key |> String.to_atom()
+  defp result_key(dimension), do: dimension

  defp maybe_add_time_on_page(event_results, site, query, metrics) do


I guess this means we won't be able to order by time_on_page?

Yes. If needed, I think we can restore this but given we have a time_on_page project in plans I can live with this limitation.

ukutaht · 2024-06-27T09:13:15Z

lib/plausible/stats/filters/where_builder.ex

@@ -150,6 +152,15 @@ defmodule Plausible.Stats.Filters.WhereBuilder do
    true
  end

+  defp add_filter(table, _query, filter) do
+    Logger.info("Unable to process garbage filter. No results are returned",


At first glance I'm not a fan of silently accepting/ignoring garbage. Would it make sense to error out the whole query here due to invalid filter?

Not a fan either but there's an existing test that tested against sentry spam when security scanners are run against our API: test/plausible_web/controllers/api/stats_controller/conversions_test.exs:211.

Not sure if this is indeed the best solution - do you recommend keeping this, removing the test or something else.

do you recommend keeping this, removing the test or something else.

Can we make it a 400 Bad Request rather than 200 OK?

lib/plausible/stats/query_result.ex

ukutaht · 2024-06-27T09:18:07Z

lib/plausible/stats/sql/query_builder.ex

-    Enum.reduce(query.order_by || [], q, &build_order_by(&2, query, &1, mode))
+    q
+    |> select_merge(^%{key => Expression.dimension(dimension, query, key)})
+    |> group_by([], selected_as(^key))


This reverts commit b442bb5.

…work

Previous behavior relied on two queries being made - new query leads to 0 naturally

Its hard to fit into the new schema and likely needs a rethink for apiv2

Most failures are related to ordering, pageviews shouldnt be read off of sessions

Pageviews were incorrectly fetched from sessions table before, causing issues

macobo force-pushed the apiv2-breakdown-module-remove branch from 2638495 to 41bf62e Compare June 27, 2024 08:06

macobo force-pushed the apiv2-aggregates-tests branch from 67aa08f to b442bb5 Compare June 27, 2024 08:11

macobo force-pushed the apiv2-breakdown-module-remove branch from 4724dfe to 763d449 Compare June 27, 2024 08:12

macobo marked this pull request as ready for review June 27, 2024 08:26

macobo requested a review from ukutaht June 27, 2024 08:26

macobo force-pushed the apiv2-breakdown-module-remove branch 2 times, most recently from 5137a74 to f49ccbd Compare June 27, 2024 09:04

macobo changed the title ~~APIv2: Use QueryBuilder in breakdown module.~~ APIv2: Replace breakdown module with QueryBuilder Jun 27, 2024

ukutaht reviewed Jun 27, 2024

View reviewed changes

macobo added the deploy-to-staging Special label that triggers a deploy to a staging environment label Jun 27, 2024

Base automatically changed from apiv2-aggregates-tests to master June 28, 2024 05:59

macobo added 19 commits June 28, 2024 09:45

WIP: Breakdown using QueryBuilder

719a800

Revert "Remove problematic test"

c2d235e

This reverts commit b442bb5.

Get more breakdown tests passing

2826624

Preload goals, sort when dealing with time_on_page

3fe8155

Handle conversion_rate in breakdowns

4aa91db

Simplify ordering by using selected_as consistently for dimensions

774fe43

Get breakdown tests passing

2e00452

Strings to atoms in keys for StatsController.transform_keys calls to …

c461788

…work

Handle revenue metrics removal

8e026fa

Add test for nil-removal case

7c80afa

Include percentage metric

29908e0

Fix and test with imported locations

6422ad5

Fixup time-on-page

3e1bb3e

Fix country/region automatic filters

91138e2

Handle multiple imports (os/browser version) in importsv2

f23d8c0

Filter goals

44be971

Default to ordering by page as well

411abb3

Calculate conversion rate on sessions if needed

e6e033b

Order by event dimensions - handles event:page special case

18f5e20

macobo added 26 commits June 28, 2024 09:45

Update tests

eec2093

Update more tests, handle goal=0 case in imports

3cd2613

Handle event:goal breakdowns correctly with filters

f53a95f

Revenue to money

03d29fe

Improved table deciding

4b17275

Also update event:page filters on event:page breakdown

a61cceb

bounce_rate to 0

08a2a5c

Previous behavior relied on two queries being made - new query leads to 0 naturally

Update pagination test

d464cfd

dont count non-pageviews as path goal completions

d25fc3b

Make revenue logic breakdown-specific

0b38f12

Its hard to fit into the new schema and likely needs a rethink for apiv2

Retain previous behavior for TimeSeries module

a0bef27

Get GA4 test passing

7e3c940

Most failures are related to ordering, pageviews shouldnt be read off of sessions

Clean up old methods

f402f66

Simplify imported.ex

a1a1d10

Dont crash on garbage filters

98dd4c2

Reflect ordering-related change in test

f27010c

Fix test data

34025d9

Update table_decider

1b5b00b

Re-simplify get_revenue_tracking_currency

bbab7a1

Revert revenue changes

d9a7de5

Use Query.set

6372eec

Remove a TODO

f8b0765

csv importer: no pageviews

43480c4

Pageviews were incorrectly fetched from sessions table before, causing issues

csv importer tweaking

64a6472

Remove use Plausible

7abac8e

to_existing_atom

eac23c8

macobo force-pushed the apiv2-breakdown-module-remove branch from 139e3ee to eac23c8 Compare June 28, 2024 06:45

ukutaht approved these changes Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APIv2: Replace breakdown module with QueryBuilder #4283

APIv2: Replace breakdown module with QueryBuilder #4283

macobo commented Jun 27, 2024 •

edited

Loading

ukutaht Jun 27, 2024

macobo Jun 27, 2024

ukutaht Jun 27, 2024

macobo Jun 27, 2024

ukutaht Jun 28, 2024

ukutaht Jun 27, 2024

APIv2: Replace breakdown module with QueryBuilder #4283

Are you sure you want to change the base?

APIv2: Replace breakdown module with QueryBuilder #4283

Conversation

macobo commented Jun 27, 2024 • edited Loading

Changes

ukutaht Jun 27, 2024

Choose a reason for hiding this comment

macobo Jun 27, 2024

Choose a reason for hiding this comment

ukutaht Jun 27, 2024

Choose a reason for hiding this comment

macobo Jun 27, 2024

Choose a reason for hiding this comment

ukutaht Jun 28, 2024

Choose a reason for hiding this comment

ukutaht Jun 27, 2024

Choose a reason for hiding this comment

macobo commented Jun 27, 2024 •

edited

Loading