diff --git a/CHANGELOG.md b/CHANGELOG.md index be54a7a5153a..ae0f96901a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ All notable changes to this project will be documented in this file. - Sources like 'google' and 'facebook' are now stored in capitalized forms ('Google', 'Facebook') plausible/analytics#4417 - `DATABASE_CACERTFILE` now forces TLS for PostgreSQL connections, so you don't need to add `?ssl=true` in `DATABASE_URL` - Change auth session cookies to token-based ones with server-side expiration management. +- Improve Google error messages in CE plausible/analytics#4485 ### Fixed diff --git a/fixture/ga4_api_disabled_error.json b/fixture/ga4_api_disabled_error.json new file mode 100644 index 000000000000..d394180cf0d8 --- /dev/null +++ b/fixture/ga4_api_disabled_error.json @@ -0,0 +1,27 @@ +{ + "error": { + "code": 403, + "details": [ + { + "@type": "type.googleapis.com/google.rpc.Help", + "links": [ + { + "description": "Google developers console API activation", + "url": "https://console.developers.google.com/apis/api/analyticsadmin.googleapis.com/overview?project=752168887897" + } + ] + }, + { + "@type": "type.googleapis.com/google.rpc.ErrorInfo", + "domain": "googleapis.com", + "metadata": { + "consumer": "projects/752168887897", + "service": "analyticsadmin.googleapis.com" + }, + "reason": "SERVICE_DISABLED" + } + ], + "message": "Google Analytics Admin API has not been used in project 752168887897 before or it is disabled. Enable it by visiting https://console.developers.google.com/apis/api/analyticsadmin.googleapis.com/overview?project=752168887897 then retry. If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.", + "status": "PERMISSION_DENIED" + } +} diff --git a/lib/plausible/google/ga4/http.ex b/lib/plausible/google/ga4/http.ex index 012fabb61e5e..68533a917a64 100644 --- a/lib/plausible/google/ga4/http.ex +++ b/lib/plausible/google/ga4/http.ex @@ -3,6 +3,8 @@ defmodule Plausible.Google.GA4.HTTP do HTTP client implementation for Google Analytics 4 API. """ + use Plausible + alias Plausible.HTTPClient require Logger @@ -55,7 +57,9 @@ defmodule Plausible.Google.GA4.HTTP do {:ok, report} <- convert_to_maps(report) do {:ok, {report, row_count}} else - {:error, %{reason: %{status: 429, body: body}}} -> + {:error, %{reason: %{status: 429, body: body}} = error} -> + log_ce_error("retrieving report for #{report_request.dataset}", error) + Logger.debug( "[#{inspect(__MODULE__)}:#{report_request.property}] Request failed for #{report_request.dataset} due to exceeding rate limit." ) @@ -65,7 +69,9 @@ defmodule Plausible.Google.GA4.HTTP do {:error, {:rate_limit_exceeded, dataset: report_request.dataset, offset: report_request.offset}} - {:error, %{reason: %{status: status, body: body}}} -> + {:error, %{reason: %{status: status, body: body}} = error} -> + log_ce_error("retrieving report for #{report_request.dataset}", error) + Logger.debug( "[#{inspect(__MODULE__)}:#{report_request.property}] Request failed for #{report_request.dataset} with code #{status}: #{inspect(body)}" ) @@ -74,6 +80,8 @@ defmodule Plausible.Google.GA4.HTTP do {:error, :request_failed} {:error, reason} -> + log_ce_error("retrieving report for #{report_request.dataset}", reason) + Logger.debug( "[#{inspect(__MODULE__)}:#{report_request.property}] Request failed for #{report_request.dataset}: #{inspect(reason)}" ) @@ -152,16 +160,20 @@ defmodule Plausible.Google.GA4.HTTP do {:ok, %Finch.Response{body: body, status: 200}} -> {:ok, body} - {:error, %HTTPClient.Non200Error{reason: %{status: 429}}} -> + {:error, %HTTPClient.Non200Error{reason: %{status: 429}} = error} -> + log_ce_error("listing accounts for user", error) {:error, :rate_limit_exceeded} {:error, %HTTPClient.Non200Error{} = error} when error.reason.status in [401, 403] -> - {:error, :authentication_failed} + log_ce_error("listing accounts for user", error) + {:error, authentication_failed(error)} - {:error, %{reason: :timeout}} -> + {:error, %{reason: :timeout} = error} -> + log_ce_error("listing accounts for user", error) {:error, :timeout} {:error, error} -> + log_ce_error("listing accounts for user", error) Sentry.capture_message("Error listing GA4 accounts for user", extra: %{error: error}) {:error, :unknown} end @@ -176,19 +188,25 @@ defmodule Plausible.Google.GA4.HTTP do {:ok, %Finch.Response{body: body, status: 200}} -> {:ok, body} - {:error, %HTTPClient.Non200Error{reason: %{status: 429}}} -> + {:error, %HTTPClient.Non200Error{reason: %{status: 429}} = error} -> + log_ce_error("retrieving property #{property}", error) {:error, :rate_limit_exceeded} {:error, %HTTPClient.Non200Error{} = error} when error.reason.status in [401, 403] -> - {:error, :authentication_failed} + log_ce_error("retrieving property #{property}", error) + {:error, authentication_failed(error)} {:error, %HTTPClient.Non200Error{} = error} when error.reason.status in [404] -> + log_ce_error("retrieving property #{property}", error) {:error, :not_found} - {:error, %{reason: :timeout}} -> + {:error, %{reason: :timeout} = error} -> + log_ce_error("retrieving property #{property}", error) {:error, :timeout} {:error, error} -> + log_ce_error("retrieving property #{property}", error) + Sentry.capture_message("Error retrieving GA4 property #{property}", extra: %{error: error} ) @@ -245,16 +263,21 @@ defmodule Plausible.Google.GA4.HTTP do {:ok, date} - {:error, %HTTPClient.Non200Error{reason: %{status: 429}}} -> + {:error, %HTTPClient.Non200Error{reason: %{status: 429}} = error} -> + log_ce_error("retrieving #{edge} date", error) {:error, :rate_limit_exceeded} {:error, %HTTPClient.Non200Error{} = error} when error.reason.status in [401, 403] -> - {:error, :authentication_failed} + log_ce_error("retrieving #{edge} date", error) + {:error, authentication_failed(error)} - {:error, %{reason: :timeout}} -> + {:error, %{reason: :timeout} = error} -> + log_ce_error("retrieving #{edge} date", error) {:error, :timeout} {:error, error} -> + log_ce_error("retrieving #{edge} date", error) + Sentry.capture_message("Error retrieving GA4 #{edge} date", extra: %{error: error} ) @@ -265,4 +288,29 @@ defmodule Plausible.Google.GA4.HTTP do defp reporting_api_url, do: "https://analyticsdata.googleapis.com" defp admin_api_url, do: "https://analyticsadmin.googleapis.com" + + @spec authentication_failed(HTTPClient.Non200Error.t()) :: + {:authentication_failed, String.t() | nil} + defp authentication_failed(error) do + message = + case error.reason.body do + %{"error" => %{"message" => message}} when is_binary(message) -> message + _ -> nil + end + + {:authentication_failed, message} + end + + @spec log_ce_error(String.t(), any) :: :ok + defp log_ce_error(action, error) + + on_ce do + defp log_ce_error(action, error) do + Logger.error("Google Analytics 4: Failed when #{action}. Reason: #{inspect(error)}") + end + end + + on_ee do + defp log_ce_error(_action, _error), do: :ok + end end diff --git a/lib/plausible_web/controllers/google_analytics_controller.ex b/lib/plausible_web/controllers/google_analytics_controller.ex index 8e5d6592ac69..9c48da70888c 100644 --- a/lib/plausible_web/controllers/google_analytics_controller.ex +++ b/lib/plausible_web/controllers/google_analytics_controller.ex @@ -57,12 +57,20 @@ defmodule PlausibleWeb.GoogleAnalyticsController do ) |> redirect(external: redirect_route) - {:error, :authentication_failed} -> - conn - |> put_flash( - :error, + {:error, {:authentication_failed, message}} -> + default_message = "We were unable to authenticate your Google Analytics account. Please check that you have granted us permission to 'See and download your Google Analytics data' and try again." - ) + + message = + if Plausible.ce?() do + message || default_message + else + default_message + end + + conn + |> put_flash(:ttl, :timer.seconds(5)) + |> put_flash(:error, message) |> redirect(external: redirect_route) {:error, :timeout} -> @@ -128,12 +136,20 @@ defmodule PlausibleWeb.GoogleAnalyticsController do ) |> redirect(external: redirect_route) - {:error, :authentication_failed} -> - conn - |> put_flash( - :error, + {:error, {:authentication_failed, message}} -> + default_message = "Google Analytics authentication seems to have expired. Please try again." - ) + + message = + if Plausible.ce?() do + message || default_message + else + default_message + end + + conn + |> put_flash(:ttl, :timer.seconds(5)) + |> put_flash(:error, message) |> redirect(external: redirect_route) {:error, :timeout} -> @@ -192,12 +208,20 @@ defmodule PlausibleWeb.GoogleAnalyticsController do ) |> redirect(external: redirect_route) - {:error, :authentication_failed} -> - conn - |> put_flash( - :error, + {:error, {:authentication_failed, message}} -> + default_message = "Google Analytics authentication seems to have expired. Please try again." - ) + + message = + if Plausible.ce?() do + message || default_message + else + default_message + end + + conn + |> put_flash(:ttl, :timer.seconds(5)) + |> put_flash(:error, message) |> redirect(external: redirect_route) {:error, :timeout} -> diff --git a/test/plausible/imported/google_analytics4_test.exs b/test/plausible/imported/google_analytics4_test.exs index 728378787ed8..29624b8f283b 100644 --- a/test/plausible/imported/google_analytics4_test.exs +++ b/test/plausible/imported/google_analytics4_test.exs @@ -25,6 +25,10 @@ defmodule Plausible.Imported.GoogleAnalytics4Test do |> Enum.map(&File.read!/1) |> Enum.map(&Jason.decode!/1) + if Plausible.ce?() do + @moduletag :capture_log + end + setup :verify_on_exit! describe "parse_args/1 and import_data/2" do diff --git a/test/plausible_web/controllers/google_analytics_controller_test.exs b/test/plausible_web/controllers/google_analytics_controller_test.exs index 7bd79860171c..7473ee7acab2 100644 --- a/test/plausible_web/controllers/google_analytics_controller_test.exs +++ b/test/plausible_web/controllers/google_analytics_controller_test.exs @@ -10,6 +10,10 @@ defmodule PlausibleWeb.GoogleAnalyticsControllerTest do require Plausible.Imported.SiteImport + if Plausible.ce?() do + @moduletag :capture_log + end + setup :verify_on_exit! describe "GET /:website/import/google-analytics/property" do @@ -101,6 +105,43 @@ defmodule PlausibleWeb.GoogleAnalyticsControllerTest do "We were unable to authenticate your Google Analytics account" end + @tag :ce_build_only + test "redirects to imports and exports on disabled API with flash error", %{ + conn: conn, + site: site + } do + expect( + Plausible.HTTPClient.Mock, + :get, + fn _url, _opts -> + body = "fixture/ga4_api_disabled_error.json" |> File.read!() |> Jason.decode!() + {:error, %HTTPClient.Non200Error{reason: %{status: 403, body: body}}} + end + ) + + conn = + conn + |> get("/#{site.domain}/import/google-analytics/property", %{ + "access_token" => "token", + "refresh_token" => "foo", + "expires_at" => "2022-09-22T20:01:37.112777" + }) + + assert redirected_to(conn, 302) == + PlausibleWeb.Router.Helpers.site_path( + conn, + :settings_imports_exports, + site.domain + ) + + assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ + """ + Google Analytics Admin API has not been used in project 752168887897 before or it is disabled. \ + Enable it by visiting https://console.developers.google.com/apis/api/analyticsadmin.googleapis.com/overview?project=752168887897 then retry. \ + If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.\ + """ + end + test "redirects to imports and exports on timeout error with flash error", %{ conn: conn, site: site @@ -391,6 +432,44 @@ defmodule PlausibleWeb.GoogleAnalyticsControllerTest do "Google Analytics authentication seems to have expired." end + @tag :ce_build_only + test "redirects to imports and exports on disabled API with flash error", %{ + conn: conn, + site: site + } do + expect( + Plausible.HTTPClient.Mock, + :post, + fn _url, _opts, _params -> + body = "fixture/ga4_api_disabled_error.json" |> File.read!() |> Jason.decode!() + {:error, %HTTPClient.Non200Error{reason: %Finch.Response{status: 403, body: body}}} + end + ) + + conn = + conn + |> post("/#{site.domain}/import/google-analytics/property", %{ + "property" => "properties/428685906", + "access_token" => "token", + "refresh_token" => "foo", + "expires_at" => "2022-09-22T20:01:37.112777" + }) + + assert redirected_to(conn, 302) == + PlausibleWeb.Router.Helpers.site_path( + conn, + :settings_imports_exports, + site.domain + ) + + assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ + """ + Google Analytics Admin API has not been used in project 752168887897 before or it is disabled. \ + Enable it by visiting https://console.developers.google.com/apis/api/analyticsadmin.googleapis.com/overview?project=752168887897 then retry. \ + If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.\ + """ + end + test "redirects to imports and exports on timeout with flash error", %{ conn: conn, @@ -573,6 +652,47 @@ defmodule PlausibleWeb.GoogleAnalyticsControllerTest do "Google Analytics authentication seems to have expired." end + @tag :ce_build_only + test "redirects to imports and exports on disabled API with flash error", + %{ + conn: conn, + site: site + } do + expect( + Plausible.HTTPClient.Mock, + :get, + fn _url, _params -> + body = "fixture/ga4_api_disabled_error.json" |> File.read!() |> Jason.decode!() + {:error, %HTTPClient.Non200Error{reason: %{status: 403, body: body}}} + end + ) + + conn = + conn + |> get("/#{site.domain}/import/google-analytics/confirm", %{ + "property" => "properties/428685906", + "access_token" => "token", + "refresh_token" => "foo", + "expires_at" => "2022-09-22T20:01:37.112777", + "start_date" => "2024-02-22", + "end_date" => "2024-02-26" + }) + + assert redirected_to(conn, 302) == + PlausibleWeb.Router.Helpers.site_path( + conn, + :settings_imports_exports, + site.domain + ) + + assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ + """ + Google Analytics Admin API has not been used in project 752168887897 before or it is disabled. \ + Enable it by visiting https://console.developers.google.com/apis/api/analyticsadmin.googleapis.com/overview?project=752168887897 then retry. \ + If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.\ + """ + end + test "redirects to imports and exports on timeout with flash error", %{ conn: conn,