-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
APIv2: Replace breakdown module with QueryBuilder #4283
base: master
Are you sure you want to change the base?
Conversation
2638495
to
41bf62e
Compare
67aa08f
to
b442bb5
Compare
4724dfe
to
763d449
Compare
5137a74
to
f49ccbd
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this means we won't be able to order by time_on_page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you recommend keeping this, removing the test or something else.
Can we make it a 400 Bad Request rather than 200 OK?
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
This reverts commit b442bb5.
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
139e3ee
to
eac23c8
Compare
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:
time_on_page
is a special case within the breakdown module.Depends on #4251