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.

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

Scroll depth dashboard warnings (imported data) #5051

Merged
merged 21 commits into from
Feb 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion assets/js/dashboard/components/sort-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ export const SortButton = ({
return (
<button
onClick={toggleSort}
title={next.hint}
className={classNames('group', 'hover:underline', 'relative')}
>
{children}
<span
title={next.hint}
className={classNames(
'absolute',
'rounded inline-block h-4 w-4',
Expand Down
30 changes: 28 additions & 2 deletions assets/js/dashboard/components/table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import classNames from 'classnames'
import React, { ReactNode } from 'react'
import { SortDirection } from '../hooks/use-order-by'
import { SortButton } from './sort-button'
import { Tooltip } from '../util/tooltip'

export type ColumnConfiguraton<T extends Record<string, unknown>> = {
/** Unique column ID, used for sorting purposes and to get the value of the cell using rowItem[key] */
Expand All @@ -17,6 +18,8 @@ export type ColumnConfiguraton<T extends Record<string, unknown>> = {
width: string
/** Aligns column content. */
align?: 'left' | 'right'
/** A warning to be rendered as a tooltip for the column header */
metricWarning?: string
/**
* Function used to transform the value found at item[key] for the cell. Superseded by renderItem if present. @example 1120 => "1.1k"
*/
Expand Down Expand Up @@ -100,6 +103,29 @@ export const Table = <T extends Record<string, string | number | ReactNode>>({
columns: ColumnConfiguraton<T>[]
data: T[] | { pages: T[][] }
}) => {
const renderColumnLabel = (column: ColumnConfiguraton<T>) => {
if (column.metricWarning) {
return (
<Tooltip
info={warningSpan(column.metricWarning)}
className="inline-block"
>
{column.label + ' *'}
</Tooltip>
)
} else {
return column.label
}
}

const warningSpan = (warning: string) => {
return (
<span className="text-xs font-normal whitespace-nowrap">
{'*' + warning}
</span>
)
}

return (
<table className="w-max overflow-x-auto md:w-full table-striped table-fixed">
<thead>
Expand All @@ -115,10 +141,10 @@ export const Table = <T extends Record<string, string | number | ReactNode>>({
toggleSort={column.onSort}
sortDirection={column.sortDirection ?? null}
>
{column.label}
{renderColumnLabel(column)}
</SortButton>
) : (
column.label
renderColumnLabel(column)
)}
</TableHeaderCell>
))}
Expand Down
1 change: 1 addition & 0 deletions assets/js/dashboard/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export type DashboardQuery = typeof queryDefaultValue
export type BreakdownResultMeta = {
date_range_label: string
comparison_date_range_label?: string
metric_warnings: Record<string, Record<string, string>> | undefined
}

export function addFilter(
Expand Down
9 changes: 9 additions & 0 deletions assets/js/dashboard/stats/graph/top-stats.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) {
<SecondsSinceLastLoad lastLoadTimestamp={lastLoadTimestamp} />s ago
</p>
)}

{stat.name === 'Scroll depth' &&
RobertJoonas marked this conversation as resolved.
Show resolved Hide resolved
data.meta.metric_warnings?.scroll_depth?.code ===
'no_imported_scroll_depth' && (
<p className="font-normal text-xs whitespace-nowrap">
* Does not include imported data
</p>
)}
</div>
)
}
Expand Down Expand Up @@ -115,6 +123,7 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) {
{statExtraName && (
<span className="hidden sm:inline-block ml-1">{statExtraName}</span>
)}
{stat.warning_code && <span className="inline-block ml-1">*</span>}
</div>
)
}
Expand Down
13 changes: 13 additions & 0 deletions assets/js/dashboard/stats/modals/breakdown-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ export default function BreakdownModal<TListItem extends { name: string }>({
key: m.key,
width: m.width,
align: 'right',
metricWarning: getMetricWarning(m, meta),
renderValue: (item) => m.renderValue(item, meta),
onSort: m.sortable ? () => toggleSortByMetric(m) : undefined,
sortDirection: orderByDictionary[m.key]
Expand Down Expand Up @@ -233,3 +234,15 @@ const ExternalLinkIcon = ({ url }: { url?: string }) =>
</svg>
</a>
) : null

const getMetricWarning = (metric: Metric, meta: BreakdownResultMeta | null) => {
const warnings = meta?.metric_warnings

if (warnings) {
const code = warnings[metric.key]?.code

if (code == 'no_imported_scroll_depth') {
return 'Does not include imported data'
}
}
}
69 changes: 50 additions & 19 deletions lib/plausible/data_migration/site_imports.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,51 @@ defmodule Plausible.DataMigration.SiteImports do

import Ecto.Query

alias Plausible.ClickhouseRepo
alias Plausible.Imported
alias Plausible.Imported.SiteImport
alias Plausible.Repo
alias Plausible.Site
alias Plausible.{Repo, ClickhouseRepo, Site}

require Plausible.Imported.SiteImport
defmodule SiteImportSnapshot do
@moduledoc """
A snapshot of the Plausible.Imported.SiteImport schema from April 2024.
"""

use Ecto.Schema

schema "site_imports" do
field :start_date, :date
field :end_date, :date
field :label, :string
field :source, Ecto.Enum, values: [:universal_analytics, :google_analytics_4, :csv, :noop]
field :status, Ecto.Enum, values: [:pending, :importing, :completed, :failed]
field :legacy, :boolean, default: false

belongs_to :site, Plausible.Site
belongs_to :imported_by, Plausible.Auth.User
RobertJoonas marked this conversation as resolved.
Show resolved Hide resolved

timestamps()
end
end

@imported_tables_april_2024 [
"imported_visitors",
"imported_sources",
"imported_pages",
"imported_entry_pages",
"imported_exit_pages",
"imported_custom_events",
"imported_locations",
"imported_devices",
"imported_browsers",
"imported_operating_systems"
]

def imported_tables_april_2024(), do: @imported_tables_april_2024

def run(opts \\ []) do
dry_run? = Keyword.get(opts, :dry_run?, true)

site_import_query =
from(i in Imported.SiteImport,
where: i.site_id == parent_as(:site).id and i.status == ^SiteImport.completed(),
from(i in SiteImportSnapshot,
where: i.site_id == parent_as(:site).id and i.status == ^:completed,
select: 1
)

Expand All @@ -37,7 +68,7 @@ defmodule Plausible.DataMigration.SiteImports do
|> Repo.all(log: false)

site_imports =
from(i in Imported.SiteImport, where: i.status == ^SiteImport.completed())
from(i in SiteImportSnapshot, where: i.status == ^:completed)
|> Repo.all(log: false)

legacy_site_imports = backfill_legacy_site_imports(sites_with_only_legacy_import, dry_run?)
Expand All @@ -64,7 +95,7 @@ defmodule Plausible.DataMigration.SiteImports do
|> Map.put(:site_id, site.id)
|> Map.take([:legacy, :start_date, :end_date, :source, :status, :site_id])

%Imported.SiteImport{}
%SiteImportSnapshot{}
|> Ecto.Changeset.change(params)
|> insert!(dry_run?)
end
Expand Down Expand Up @@ -146,11 +177,11 @@ defmodule Plausible.DataMigration.SiteImports do
# Exposed for testing purposes
@doc false
def imported_stats_end_date(site_id, import_ids) do
[first_schema | schemas] = Imported.schemas()
[first_table | tables] = @imported_tables_april_2024

query =
Enum.reduce(schemas, max_date_query(first_schema, site_id, import_ids), fn schema, query ->
from(s in subquery(union_all(query, ^max_date_query(schema, site_id, import_ids))))
Enum.reduce(tables, max_date_query(first_table, site_id, import_ids), fn table, query ->
from(s in subquery(union_all(query, ^max_date_query(table, site_id, import_ids))))
end)

dates = ClickhouseRepo.all(from(q in query, select: q.max_date), log: false)
Expand Down Expand Up @@ -211,8 +242,8 @@ defmodule Plausible.DataMigration.SiteImports do
entity
end

defp max_date_query(schema, site_id, import_ids) do
from(q in schema,
defp max_date_query(table, site_id, import_ids) do
from(q in table,
where: q.site_id == ^site_id,
where: q.import_id in ^import_ids,
select: %{max_date: fragment("max(?)", q.date)}
Expand All @@ -222,12 +253,12 @@ defmodule Plausible.DataMigration.SiteImports do
defp from_legacy(%Site.ImportedData{} = data) do
status =
case data.status do
"ok" -> SiteImport.completed()
"error" -> SiteImport.failed()
_ -> SiteImport.importing()
"ok" -> :completed
"error" -> :failed
_ -> :importing
end

%SiteImport{
%SiteImportSnapshot{
id: 0,
legacy: true,
start_date: data.start_date,
Expand Down
16 changes: 13 additions & 3 deletions lib/plausible/imported.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@ defmodule Plausible.Imported do

import Ecto.Query

alias Plausible.Imported
alias Plausible.{Site, Repo, Imported}
alias Plausible.Imported.SiteImport
alias Plausible.Repo
alias Plausible.Site
alias Plausible.Stats.Query

require Plausible.Imported.SiteImport

Expand Down Expand Up @@ -97,6 +96,17 @@ 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
date_range = Query.date_range(query)

site
|> get_completed_imports()
|> Enum.filter(fn site_import ->
site_import.start_date <= date_range.last && site_import.end_date >= date_range.first
end)
end

@spec get_import(Site.t(), non_neg_integer()) :: SiteImport.t() | nil
def get_import(site, import_id) do
Repo.get_by(SiteImport, id: import_id, site_id: site.id)
Expand Down
21 changes: 21 additions & 0 deletions lib/plausible/imported/csv_importer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ defmodule Plausible.Imported.CSVImporter do
"""

use Plausible.Imported.Importer
import Ecto.Query, only: [from: 2]

@impl true
def name(), do: :csv
Expand Down Expand Up @@ -54,6 +55,26 @@ defmodule Plausible.Imported.CSVImporter do
{:error, Exception.message(e)}
end

def on_success(site_import, _extra_data) do
has_scroll_depth? =
Plausible.ClickhouseRepo.exists?(
from(i in "imported_pages",
where: i.site_id == ^site_import.site_id,
where: i.import_id == ^site_import.id,
where: not is_nil(i.scroll_depth),
select: 1
)
)

if has_scroll_depth? do
site_import
|> Ecto.Changeset.change(%{has_scroll_depth: true})
|> Plausible.Repo.update!()
end

:ok
end

defp import_s3(ch, site_import, uploads) do
%{
id: import_id,
Expand Down
1 change: 1 addition & 0 deletions lib/plausible/imported/site_import.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ defmodule Plausible.Imported.SiteImport do
field :source, Ecto.Enum, values: ImportSources.names()
field :status, Ecto.Enum, values: @statuses
field :legacy, :boolean, default: false
field :has_scroll_depth, :boolean, default: false

belongs_to :site, Site
belongs_to :imported_by, User
Expand Down
37 changes: 31 additions & 6 deletions lib/plausible/stats/query_result.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ defmodule Plausible.Stats.QueryResult do

use Plausible
alias Plausible.Stats.{DateTimeRange, Query, QueryRunner}
alias Plausible.Imported

defstruct results: [],
meta: %{},
Expand Down Expand Up @@ -89,8 +90,8 @@ defmodule Plausible.Stats.QueryResult do
end
end

defp add_metric_warnings_meta(meta, %QueryRunner{main_query: query}) do
warnings = metric_warnings(query)
defp add_metric_warnings_meta(meta, runner) do
warnings = metric_warnings(meta, runner)

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

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

defp metric_warning(metric, query) when metric in @revenue_metrics do
defp metric_warning(metric, _meta, %QueryRunner{main_query: query})
when metric in @revenue_metrics do
if query.revenue_warning do
%{
code: query.revenue_warning,
Expand All @@ -162,7 +164,30 @@ defmodule Plausible.Stats.QueryResult do
end
end

defp metric_warning(_metric, _query), do: nil
@no_imported_scroll_depth_metric_warning %{
code: :no_imported_scroll_depth,
warning: "No imports with scroll depth data were found"
}

defp metric_warning(:scroll_depth, %{imports_included: true} = _meta, %QueryRunner{} = runner) do
RobertJoonas marked this conversation as resolved.
Show resolved Hide resolved
%QueryRunner{site: site, main_query: main_query, comparison_query: comparison_query} = runner

imports_in_main_range = Imported.completed_imports_in_query_range(site, main_query)
RobertJoonas marked this conversation as resolved.
Show resolved Hide resolved

imports_in_comparison_range =
case comparison_query do
nil -> []
%Query{} = query -> Imported.completed_imports_in_query_range(site, query)
end

imports_in_range = imports_in_main_range ++ imports_in_comparison_range

if not Enum.any?(imports_in_range, & &1.has_scroll_depth) do
@no_imported_scroll_depth_metric_warning
end
end

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

defp to_iso8601(datetime, timezone) do
datetime
Expand Down
Loading
Loading