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

@@ -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
 

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

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

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

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