Skip to content
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.

Sign up for GitHub

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

Hide dashboard import icons when redundant #2 #5102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+124 −109
Open

Hide dashboard import icons when redundant #2 #5102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61379a4
Include comparison query on main query after optimize
macobo Feb 20, 2025
421bf32
Refactor how query.include_imported works and related query attribute…
macobo Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions lib/plausible/imported.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,22 +66,24 @@ defmodule Plausible.Imported do
@goals_with_path
end

@spec any_completed_imports?(Site.t()) :: boolean()
def any_completed_imports?(site) do
get_completed_imports(site) != []
@spec completed_imports(Site.t()) :: [SiteImport.t()]
def completed_imports(site) do
site
|> Repo.preload(:completed_imports)
|> Map.fetch!(:completed_imports)
end

@spec earliest_import_start_date(Site.t()) :: Date.t() | nil
def earliest_import_start_date(site) do
site
|> get_completed_imports()
|> completed_imports()
|> Enum.map(& &1.start_date)
|> Enum.min(Date, fn -> nil end)
end

@spec complete_import_ids(Site.t()) :: [non_neg_integer()]
def complete_import_ids(site) do
imports = get_completed_imports(site)
imports = completed_imports(site)
has_legacy? = Enum.any?(imports, fn %{legacy: legacy?} -> legacy? end)
ids = Enum.map(imports, fn %{id: id} -> id end)

Expand All @@ -93,12 +95,11 @@ defmodule Plausible.Imported do
end
end

@spec completed_imports_in_query_range(Site.t(), Query.t()) :: [SiteImport.t()]
def completed_imports_in_query_range(%Site{} = site, %Query{} = query) do
@spec completed_imports_in_query_range(Query.t()) :: [SiteImport.t()]
def completed_imports_in_query_range(%Query{} = query) do
date_range = Query.date_range(query)

site
|> get_completed_imports()
query.completed_imports
|> Enum.reject(fn site_import ->
Date.after?(site_import.start_date, date_range.last) or
Date.before?(site_import.end_date, date_range.first)
Expand Down Expand Up @@ -218,10 +219,4 @@ defmodule Plausible.Imported do
defp in_range?(date, range) do
Date.before?(range.first, date) && Date.after?(range.last, date)
end

defp get_completed_imports(site) do
site
|> Repo.preload(:completed_imports)
|> Map.fetch!(:completed_imports)
end
end
1 change: 1 addition & 0 deletions lib/plausible/stats/aggregate.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ defmodule Plausible.Stats.Aggregate do
query =
query
|> Query.set(metrics: metrics, remove_unavailable_revenue_metrics: true)
|> Query.set_include(:dashboard_imports_meta, true)
|> QueryOptimizer.optimize()

%QueryResult{results: [entry], meta: meta} = QueryRunner.run(site, query)
Expand Down
2 changes: 1 addition & 1 deletion lib/plausible/stats/filters/query_parser.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ defmodule Plausible.Stats.Filters.QueryParser do
# is false. Even if we don't want to include imported data, we
# might still want to know whether imported data can be toggled
# on/off on the dashboard.
imports_meta: false,
dashboard_imports_meta: false,
time_labels: false,
total_rows: false,
comparisons: nil
Expand Down
59 changes: 26 additions & 33 deletions lib/plausible/stats/query.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@ defmodule Plausible.Stats.Query do
use Plausible

defstruct utc_time_range: nil,
comparison_utc_time_range: nil,
interval: nil,
period: nil,
dimensions: [],
filters: [],
sample_threshold: 20_000_000,
imports_exist: false,
completed_imports: [],
imports_in_range: [],
include_imported: false,
skip_imported_reason: nil,
Expand All @@ -26,7 +25,9 @@ defmodule Plausible.Stats.Query do
revenue_warning: nil,
remove_unavailable_revenue_metrics: false,
site_id: nil,
site_native_stats_start_at: nil
site_native_stats_start_at: nil,
comparison_utc_time_range: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about getting rid of comparison_utc_time_range? That information might as well be included under the comparison_query itself.

comparison_query: nil

require OpenTelemetry.Tracer, as: Tracer
alias Plausible.Stats.{DateTimeRange, Filters, Imported, Legacy, Comparisons}
Expand Down Expand Up @@ -134,50 +135,42 @@ defmodule Plausible.Stats.Query do
end

def put_imported_opts(query, site) do
requested? = query.include.imports

query =
if site do
struct!(query,
imports_exist: Plausible.Imported.any_completed_imports?(site),
imports_in_range: get_imports_in_range(site, query)
)
else
query
end

skip_imported_reason = get_skip_imported_reason(query)

struct!(query,
include_imported: requested? and is_nil(skip_imported_reason),
skip_imported_reason: skip_imported_reason
query
|> struct!(
completed_imports:
if(site, do: Plausible.Imported.completed_imports(site), else: query.completed_imports)
)
|> set_imports_in_range()
|> set_imports_included()
end

defp get_imports_in_range(_site, %__MODULE__{period: period})
defp set_imports_in_range(%__MODULE__{period: period} = query)
when period in ["realtime", "30m"] do
[]
query
end

defp get_imports_in_range(site, query) do
in_range = Plausible.Imported.completed_imports_in_query_range(site, query)
defp set_imports_in_range(query) do
struct!(query,
imports_in_range: Plausible.Imported.completed_imports_in_query_range(query)
)
end

in_comparison_range =
if is_map(query.include.comparisons) do
comparison_query = Comparisons.get_comparison_query(query)
Plausible.Imported.completed_imports_in_query_range(site, comparison_query)
else
[]
end
defp set_imports_included(query) do
requested? = query.include.imports

in_comparison_range ++ in_range
skip_imported_reason = get_skip_imported_reason(query)

struct!(query,
include_imported: requested? and is_nil(skip_imported_reason),
skip_imported_reason: skip_imported_reason
)
end

@spec get_skip_imported_reason(t()) ::
nil | :no_imported_data | :out_of_range | :unsupported_query
def get_skip_imported_reason(query) do
cond do
not query.imports_exist ->
query.completed_imports == [] ->
:no_imported_data

query.imports_in_range == [] ->
Expand Down
14 changes: 12 additions & 2 deletions lib/plausible/stats/query_optimizer.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule Plausible.Stats.QueryOptimizer do
"""

use Plausible
alias Plausible.Stats.{DateTimeRange, Filters, Query, TableDecider, Util, Time}
alias Plausible.Stats.{Comparisons, DateTimeRange, Filters, Query, TableDecider, Util, Time}

@doc """
This module manipulates an existing query, updating it according to business logic.
Expand Down Expand Up @@ -49,7 +49,8 @@ defmodule Plausible.Stats.QueryOptimizer do
&add_missing_order_by/1,
&update_time_in_order_by/1,
&extend_hostname_filters_to_visit/1,
&remove_revenue_metrics_if_unavailable/1
&remove_revenue_metrics_if_unavailable/1,
&add_comparison_query/1
]
end

Expand Down Expand Up @@ -185,4 +186,13 @@ defmodule Plausible.Stats.QueryOptimizer do
else
defp remove_revenue_metrics_if_unavailable(query), do: query
end

defp add_comparison_query(query) do
if query.include.comparisons do
comparison_query = Comparisons.get_comparison_query(query)
Query.set(query, comparison_query: comparison_query)
else
query
end
end
end
43 changes: 30 additions & 13 deletions lib/plausible/stats/query_result.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,24 @@ defmodule Plausible.Stats.QueryResult do
}

defp add_imports_meta(meta, %Query{include: include} = query) do
if include.imports or include.imports_meta do
if include.imports or include.dashboard_imports_meta do
comparison_query =
query.comparison_query || %{include_imported: false, skip_imported_reason: nil}

imports_included = query.include_imported or comparison_query.include_imported

imports_skip_reason =
if(imports_included,
do: nil,
else: query.skip_imported_reason || comparison_query.skip_imported_reason
)

%{
imports_included: query.include_imported,
imports_skip_reason: query.skip_imported_reason,
imports_warning: @imports_warnings[query.skip_imported_reason]
imports_included: imports_included,
imports_skip_reason: imports_skip_reason,
imports_warning: @imports_warnings[imports_skip_reason],
imports_included_for_main_query:
if(include.dashboard_imports_meta, do: query.include_imported, else: nil)
}
|> Map.reject(fn {_key, value} -> is_nil(value) end)
|> Map.merge(meta)
Expand All @@ -72,7 +85,7 @@ defmodule Plausible.Stats.QueryResult do
end

defp add_metric_warnings_meta(meta, query) do
warnings = metric_warnings(query)
warnings = metric_warnings(meta, query)

if map_size(warnings) > 0 do
Map.put(meta, :metric_warnings, warnings)
Expand Down Expand Up @@ -111,9 +124,9 @@ defmodule Plausible.Stats.QueryResult do
end
end

defp metric_warnings(%Query{} = query) do
defp metric_warnings(meta, %Query{} = query) do
Enum.reduce(query.metrics, %{}, fn metric, acc ->
case metric_warning(metric, query) do
case metric_warning(metric, meta, query) do
nil -> acc
%{} = warning -> Map.put(acc, metric, warning)
end
Expand All @@ -132,7 +145,7 @@ defmodule Plausible.Stats.QueryResult do
"Revenue metrics are null as there are no matching revenue goals."
}

defp metric_warning(metric, %Query{} = query)
defp metric_warning(metric, _meta, %Query{} = query)
when metric in @revenue_metrics do
if query.revenue_warning do
%{
Expand All @@ -150,13 +163,17 @@ defmodule Plausible.Stats.QueryResult do
warning: "No imports with scroll depth data were found"
}

defp metric_warning(:scroll_depth, %Query{} = query) do
if query.include_imported and not Enum.any?(query.imports_in_range, & &1.has_scroll_depth) do
@no_imported_scroll_depth_metric_warning
end
defp metric_warning(:scroll_depth, %{imports_included: true}, %Query{} = query) do
[query, query.comparison_query]
|> Enum.reject(&is_nil/1)
|> Enum.find_value(fn query ->
if query.include_imported and not Enum.any?(query.imports_in_range, & &1.has_scroll_depth) do
@no_imported_scroll_depth_metric_warning
end
end)
end

defp metric_warning(_metric, _query), do: nil
defp metric_warning(_metric, _meta, _query), do: nil

defp to_iso8601(datetime, timezone) do
datetime
Expand Down
3 changes: 1 addition & 2 deletions lib/plausible/stats/query_runner.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ defmodule Plausible.Stats.QueryRunner do
defp add_comparison_query(%__MODULE__{main_query: query, main_results: main_results} = runner)
when is_map(query.include.comparisons) do
comparison_query =
query
|> Comparisons.get_comparison_query()
query.comparison_query
|> Comparisons.add_comparison_filters(main_results)

struct!(runner, comparison_query: comparison_query)
Expand Down
10 changes: 2 additions & 8 deletions lib/plausible/stats/timeseries.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ defmodule Plausible.Stats.Timeseries do
"""

use Plausible.ClickhouseRepo
alias Plausible.Stats.{Comparisons, Query, QueryRunner, QueryOptimizer, Time}
alias Plausible.Stats.{Query, QueryRunner, QueryOptimizer, Time}

@time_dimension %{
"month" => "time:month",
Expand All @@ -27,17 +27,11 @@ defmodule Plausible.Stats.Timeseries do
)
|> QueryOptimizer.optimize()

comparison_query =
if(query.include.comparisons,
do: Comparisons.get_comparison_query(query),
else: nil
)

query_result = QueryRunner.run(site, query)

{
build_result(query_result, query, fn entry -> entry end),
build_result(query_result, comparison_query, fn entry -> entry.comparison end),
build_result(query_result, query.comparison_query, fn entry -> entry.comparison end),
query_result.meta
}
end
Expand Down
24 changes: 11 additions & 13 deletions lib/plausible_web/controllers/api/stats_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,7 @@ defmodule PlausibleWeb.Api.StatsController do

params = realtime_period_to_30m(params)

query =
site
|> Query.from(params, debug_metadata(conn))
|> Query.set_include(:imports_meta, true)
query = Query.from(site, params, debug_metadata(conn))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the changes in #5097 was to provide the default "previous_period" comparison to the query here (instead of visitor-graph.js)

I think that's still required - Top Stats are always queried with a comparison option, however,

  • By default, comparison mode is previous_period, which is set in visitor-graph.js. In that "default" case. We don't want to display the imported switch / imported warnings.
  • When the user is in the comparison mode however (press X on the dashboard, other reports are showing comparison data too), this is when we do want to display the imported switch / imported warnings.

Copy link
Contributor Author

@macobo macobo Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The business logic changes you are describing in this and other comments are pretty complicated, non-intuitive and seem contradictory to what we discussed in the call. Is there a ticket somewhere with the full context?

I don't see why the frontend change is at all necessary here to fix the 'bug' that including comparison query in meta.include_imported added but I'm likely missing something.

Here's what I've got based on your original PR description:

Bug description

User opens dashboard for month of March. User has imported data in february (but none in march)

Expected result: Imports button should say "Click to include imported data"
Actual result: imports button says "Click to exclude imported data"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a ticket somewhere with the full context?

There's no ticket at the moment, as it seemed to me like a niche thing to get out of the way with a single PR. Should we create one?

I don't see why the frontend change is at all necessary here to fix the 'bug' that including comparison query in meta.include_imported added but I'm likely missing something.

The "bug" here is not very straight forward indeed as the desired behaviours are somewhat conflicting.

The Top Stats query currently serves another purpose in addition to simply aggregating the stats. It is to set the state of whether imported data is currently visible on the dashboard or not, AND whether it exists at all in the given query (with comparison) range.

From the API perspective, I wouldn't even say there's a bug - imported data is in the Top Stats comparison range so the request responds with:

  • yes - imported data WAS included
  • When the imports button (aka "with_imported_switch") is clicked, it will disable imported data globally in the dashboard, and hence the tooltip should say "click to exclude imported data"

The issue/weirdness comes from the fact that we're not in the "comparison mode" and yet the backend ends up running a comparison query because Top Stats fetches comparisons even when not in comparison mode - that is to render the small change percentages with arrows. The desired behaviour (as pointed out by Marko) is that when we're not explicitly in comparison mode, the top stats comparison range should "silently" count imported data.

The issue at hand could theoretically be solved by:

  1. Hide dashboard import icons when redundant #5097 (fixes exactly the problem I'm describing - nothing more, nothing less)
  2. not rendering the comparison change arrows UNLESS in comparison mode - I recall we did this at some point and people saw the top stats too "blank" so we put the change arrows back even in the non-comparison mode
  3. create two separate queries in the Top Stats endpoint when not in comparison mode

None of these solutions is perfect in my head.

Also, I believe this issue was there already before any imports-related work/refactoring that was part of the scroll depth project.


%{
top_stats: top_stats,
Expand All @@ -220,18 +217,19 @@ defmodule PlausibleWeb.Api.StatsController do
end

defp with_imported_switch_info(%Jason.OrderedObject{} = meta) do
case {meta[:imports_included], meta[:imports_skip_reason]} do
{true, nil} ->
%{visible: true, togglable: true, tooltip_msg: "Click to exclude imported data"}

{false, nil} ->
%{visible: true, togglable: true, tooltip_msg: "Click to include imported data"}

{false, :unsupported_query} ->
case Map.new(meta) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one case missing here:

%{imports_included_for_main_query: false, imports_included: true} ->
  %{visible: true, togglable: true, tooltip_msg: "Click to exclude imported data"}

When imports were included only in comparison range, it still means that imports were included, and the tooltip should say "click to exclude".

%{imports_included: false, imports_skip_reason: :unsupported_query} ->
%{visible: true, togglable: false, tooltip_msg: "Imported data cannot be included"}

{false, reason} when reason in [:no_imported_data, :out_of_range] ->
%{imports_included: false, imports_skip_reason: reason}
when reason in [:no_imported_data, :out_of_range] ->
%{visible: false, togglable: false, tooltip_msg: nil}

%{imports_included_for_main_query: true} ->
%{visible: true, togglable: true, tooltip_msg: "Click to exclude imported data"}

%{imports_included_for_main_query: false} ->
%{visible: true, togglable: true, tooltip_msg: "Click to include imported data"}
end
end

Expand Down
Loading
Loading