From 026cca848ea4c0a96be355e2cecf97f04f569dbd Mon Sep 17 00:00:00 2001 From: Antoine Augusti Date: Mon, 25 Sep 2023 16:36:30 +0200 Subject: [PATCH] GBFS : ne pas stocker de metrics pour 404 (#3484) * GBFS : pas de cache/telemetry pour catch-all 404 * Don't use Transport.Telemetry * Add migration to delete rows * Should stay public for now --- apps/gbfs/lib/gbfs/router.ex | 15 ++++++++-- .../controllers/index_controller_test.exs | 14 ++++++++- apps/gbfs/test/gbfs/page_cache_test.exs | 15 ---------- apps/gbfs/test/support/app_config_helper.ex | 29 ++++++++++++++----- ...925124412_gbfs_metrics_remove_404_rows.exs | 18 ++++++++++++ 5 files changed, 64 insertions(+), 27 deletions(-) create mode 100644 apps/transport/priv/repo/migrations/20230925124412_gbfs_metrics_remove_404_rows.exs diff --git a/apps/gbfs/lib/gbfs/router.ex b/apps/gbfs/lib/gbfs/router.ex index 37f03ceacc..ac0af6b78a 100644 --- a/apps/gbfs/lib/gbfs/router.ex +++ b/apps/gbfs/lib/gbfs/router.ex @@ -4,10 +4,14 @@ defmodule GBFS.Router do pipeline :api do plug(:accepts, ["json"]) plug(CORSPlug, origin: "*", credentials: false) - plug(PageCache, ttl_seconds: 30, cache_name: GBFS.Application.cache_name()) plug(TransportWeb.Plugs.AppSignalFilter) end + pipeline :page_cache do + # Cache results and send telemetry events, storing metrics + plug(PageCache, ttl_seconds: 30, cache_name: GBFS.Application.cache_name()) + end + pipeline :jcdecaux do plug(:assign_jcdecaux) end @@ -29,7 +33,7 @@ defmodule GBFS.Router do } scope "/gbfs", GBFS do - pipe_through(:api) + pipe_through([:api, :page_cache]) scope "/vcub" do get("/gbfs.json", VCubController, :index) @@ -61,10 +65,15 @@ defmodule GBFS.Router do scope "/" do pipe_through(:index_pipeline) get("/", IndexController, :index) - get("/*path", IndexController, :not_found) end end + scope "/gbfs", GBFS do + # Only the `:api` pipeline, we don't want to cache the response or send telemetry events + pipe_through(:api) + get("/*path", IndexController, :not_found) + end + defp assign_jcdecaux(conn, _) do [_, contract_id, _] = conn.path_info diff --git a/apps/gbfs/test/gbfs/controllers/index_controller_test.exs b/apps/gbfs/test/gbfs/controllers/index_controller_test.exs index 5f8d264e7d..10413a9388 100644 --- a/apps/gbfs/test/gbfs/controllers/index_controller_test.exs +++ b/apps/gbfs/test/gbfs/controllers/index_controller_test.exs @@ -1,5 +1,11 @@ defmodule GBFS.IndexControllerTest do - use GBFS.ConnCase, async: true + # This test is set to async because we need to enable the cache to test telemetry events + use GBFS.ConnCase, async: false + import AppConfigHelper + + setup do + setup_telemetry_handler() + end describe "GET /" do test "returns correct absolute urls", %{conn: conn} do @@ -19,8 +25,14 @@ defmodule GBFS.IndexControllerTest do end test "404 pages", %{conn: conn} do + # Enable PageCache + telemetry events + enable_cache() + expected_regex = ~r"^Network not found. See available data:" assert conn |> get("/gbfs/foo") |> text_response(404) =~ expected_regex assert conn |> get("/gbfs/foo/gbfs.json") |> text_response(404) =~ expected_regex + + # We did not receive telemetry events (ie metrics have not been saved to the database) + refute_receive {:telemetry_event, [:gbfs, :request, _], %{}, %{}} end end diff --git a/apps/gbfs/test/gbfs/page_cache_test.exs b/apps/gbfs/test/gbfs/page_cache_test.exs index 5e8615c8f6..2a0448f4b0 100644 --- a/apps/gbfs/test/gbfs/page_cache_test.exs +++ b/apps/gbfs/test/gbfs/page_cache_test.exs @@ -105,21 +105,6 @@ defmodule GBFS.PageCacheTest do {:telemetry_event, [:gbfs, :request, request_type], %{}, %{target: GBFS.Telemetry.target_for_network(network_name)}} end - defp setup_telemetry_handler do - events = Transport.Telemetry.gbfs_request_event_names() - events |> Enum.at(1) |> :telemetry.list_handlers() |> Enum.map(& &1.id) |> Enum.each(&:telemetry.detach/1) - test_pid = self() - # inspired by https://github.com/dashbitco/broadway/blob/main/test/broadway_test.exs - :telemetry.attach_many( - "test-handler-#{System.unique_integer()}", - events, - fn name, measurements, metadata, _ -> - send(test_pid, {:telemetry_event, name, measurements, metadata}) - end, - nil - ) - end - # To be implemented later, but for now the error handling on that (Sentry etc) # is not clear (#1378) @tag :pending diff --git a/apps/gbfs/test/support/app_config_helper.ex b/apps/gbfs/test/support/app_config_helper.ex index 70e58af6fd..7d63dcfa31 100644 --- a/apps/gbfs/test/support/app_config_helper.ex +++ b/apps/gbfs/test/support/app_config_helper.ex @@ -3,17 +3,30 @@ defmodule AppConfigHelper do A way to temporarily change Application config. Make sure to only use this with "async: false" tests """ - import ExUnit.Callbacks, only: [on_exit: 1] + def enable_cache do + change_app_config_temporarily(:gbfs, :disable_page_cache, false) + # clear the cache since some element can still be there + ExUnit.Callbacks.on_exit(fn -> Cachex.clear(:gbfs) end) + end + + def setup_telemetry_handler do + events = Enum.map([:external, :internal], &[:gbfs, :request, &1]) + events |> Enum.at(1) |> :telemetry.list_handlers() |> Enum.map(& &1.id) |> Enum.each(&:telemetry.detach/1) + test_pid = self() + # inspired by https://github.com/dashbitco/broadway/blob/main/test/broadway_test.exs + :telemetry.attach_many( + "test-handler-#{System.unique_integer()}", + events, + fn name, measurements, metadata, _ -> + send(test_pid, {:telemetry_event, name, measurements, metadata}) + end, + nil + ) + end def change_app_config_temporarily(config_name, config_key, value) do old_value = Application.fetch_env!(config_name, config_key) - on_exit(fn -> Application.put_env(config_name, config_key, old_value) end) + ExUnit.Callbacks.on_exit(fn -> Application.put_env(config_name, config_key, old_value) end) Application.put_env(config_name, config_key, value) end - - def enable_cache do - change_app_config_temporarily(:gbfs, :disable_page_cache, false) - # clear the cache since some element can still be there - on_exit(fn -> Cachex.clear(:gbfs) end) - end end diff --git a/apps/transport/priv/repo/migrations/20230925124412_gbfs_metrics_remove_404_rows.exs b/apps/transport/priv/repo/migrations/20230925124412_gbfs_metrics_remove_404_rows.exs new file mode 100644 index 0000000000..1dfad36970 --- /dev/null +++ b/apps/transport/priv/repo/migrations/20230925124412_gbfs_metrics_remove_404_rows.exs @@ -0,0 +1,18 @@ +defmodule DB.Repo.Migrations.GbfsMetricsRemove404Rows do + use Ecto.Migration + + def up do + # See https://github.com/etalab/transport-site/issues/3483 + execute """ + delete from metrics + where period >= '2023-09-15'::date and target in ( + 'gbfs:city_name_lowercase', 'gbfs:foo', + 'gbfs:valence', 'gbfs:montpellier', 'gbfs:rouen', 'gbfs:marseille', 'gbfs:strasbourg' + ) + """ + end + + def down do + IO.puts "no going back" + end +end