Fix FunctionClauseError for Finch <0.12 (#798)

Finch 0.11 and below report `telemetry` events with the same name
as those by Finch 0.12 and above, but the events carry different
meanings and the metadata map provided has a different structure.

Due to the different structure of the metadata map, our event
handlers resulted in a `FunctionClauseError`, causing the handlers
to detach.

This commit changes the event handlers so that events not matching
the expected structure are silently ignored. This means that we
do not break applications using Finch 0.11 and below, but those
Finch versions remain unsupported by our Finch integration.

.changesets/fix-functionclauseerror-for-old-finch-versions.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Fix FunctionClauseError for old Finch versions. This change explicitly ignores events from old Finch versions, meaning only Finch versions 0.12 and above will be instrumented, but using Finch versions 0.11 and below won't cause an event handler crash.

lib/appsignal/finch.ex

@@ -39,6 +39,22 @@ defmodule Appsignal.Finch do
     do_finch_request_start(@tracer.current_span(), name, request)
   end
 
+  def finch_request_start(_event, _measurements, _metadata, _config) do
+    # In Finch versions 0.11 and below, the `request` events for `start` and
+    # `stop` (but not `exception`) are also emitted, but the events' meaning is
+    # different, and the metadata object provided does not contain a `request`
+    # key.
+    # The implementations of the event handling functions above this perform
+    # pattern matching on the presence of the `request` key, as a way to
+    # check that the event shape is correct, meaning that the event is from
+    # Finch version 0.12 or above.
+    # The nil-returning catch-all implementations of these functions exist to
+    # catch events emitted by Finch versions below 0.12 and do nothing,
+    # ensuring that a `FunctionClauseError` is not raised.
+
+    nil
+  end
+
   defp do_finch_request_start(nil, _name, _request), do: nil
 
   defp do_finch_request_start(parent, _name, request) do
@@ -50,10 +66,14 @@ defmodule Appsignal.Finch do
     |> @span.set_attribute("appsignal:category", "request.finch")
   end
 
-  def finch_request_stop(_event, _measurements, _metadata, _config) do
+  def finch_request_stop(_event, _measurements, %{request: _request}, _config) do
     @tracer.close_span(@tracer.current_span())
   end
 
+  def finch_request_stop(_event, _measurements, _metadata, _config) do
+    nil
+  end
+
   def finch_request_exception(_event, _measurements, metadata, _config) do
     @tracer.current_span()
     |> @span.add_error(metadata[:kind], metadata[:reason], metadata[:stacktrace])

test/appsignal/finch_test.exs

@@ -5,6 +5,7 @@ defmodule Appsignal.FinchTest do
   test "attaches to Finch events automatically" do
     assert attached?([:finch, :request, :start])
     assert attached?([:finch, :request, :stop])
+    assert attached?([:finch, :request, :exception])
   end
 
   describe "finch_request_start/4, without a root span" do
@@ -36,6 +37,68 @@ defmodule Appsignal.FinchTest do
     end
   end
 
+  describe "finch_request_start/4, with an unsupported event shape" do
+    setup do
+      start_supervised!(Test.Nif)
+      start_supervised!(Test.Tracer)
+      start_supervised!(Test.Span)
+      start_supervised!(Test.Monitor)
+
+      :telemetry.execute(
+        [:finch, :request, :start],
+        %{},
+        # Finch 0.11 will emit events such as this, where the properties
+        # of the request are not contained in a `request` map.
+        %{
+          method: "GET",
+          scheme: :https,
+          path: "/",
+          host: "example.com",
+          port: 443
+        }
+      )
+    end
+
+    test "does not create a span" do
+      assert Test.Tracer.get(:create_span) == :error
+    end
+
+    test "does not detach the handler" do
+      assert attached?([:finch, :request, :start])
+    end
+  end
+
+  describe "finch_request_stop/4, with an unsupported event shape" do
+    setup do
+      start_supervised!(Test.Nif)
+      start_supervised!(Test.Tracer)
+      start_supervised!(Test.Span)
+      start_supervised!(Test.Monitor)
+
+      :telemetry.execute(
+        [:finch, :request, :stop],
+        %{},
+        # Finch 0.11 will emit events such as this, where the properties
+        # of the request are not contained in a `request` map.
+        %{
+          method: "GET",
+          scheme: :https,
+          path: "/",
+          host: "example.com",
+          port: 443
+        }
+      )
+    end
+
+    test "does not close a span" do
+      assert Test.Tracer.get(:close_span) == :error
+    end
+
+    test "does not detach the handler" do
+      assert attached?([:finch, :request, :stop])
+    end
+  end
+
   describe "finch_request_start/4, and finch_request_stop/4 with a root span" do
     setup do
       start_supervised!(Test.Nif)
@@ -64,7 +127,7 @@ defmodule Appsignal.FinchTest do
       :telemetry.execute(
         [:finch, :request, :stop],
         %{},
-        %{}
+        %{request: %{}}
       )
     end
 
@@ -77,7 +140,7 @@ defmodule Appsignal.FinchTest do
     end
 
     test "sets the span's category" do
-      assert attribute("appsignal:category", "request.finch")
+      assert attribute?("appsignal:category", "request.finch")
     end
 
     test "closes the span" do
@@ -117,7 +180,7 @@ defmodule Appsignal.FinchTest do
     end
   end
 
-  defp attribute(asserted_key, asserted_data) do
+  defp attribute?(asserted_key, asserted_data) do
     {:ok, attributes} = Test.Span.get(:set_attribute)
 
     Enum.any?(attributes, fn {%Span{}, key, data} ->