From 352c17231a5b479c4fdfc03eadc46f62443f2184 Mon Sep 17 00:00:00 2001
From: Ho-Yon Mak <hoyon.mak@gmail.com>
Date: Mon, 22 Nov 2021 19:08:31 +0000
Subject: [PATCH] Set error status for ecto errors in opentelemetry_ecto (#41)

* Set error status on error instead of just adding attribute

* Use Exception.message if error is an exception

Co-authored-by: Bryan Naegele <bryannaegele@users.noreply.github.com>
---
 .../lib/opentelemetry_ecto.ex                 | 37 +++++++++++--------
 .../test/opentelemetry_ecto_test.exs          | 24 +++++++++++-
 2 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex b/instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex
index f105e8b..71a7e98 100644
--- a/instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex
+++ b/instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex
@@ -67,23 +67,16 @@ defmodule OpentelemetryEcto do
         _ -> type
       end
 
-    result =
-      case query_result do
-        {:ok, _} -> []
-        _ -> [error: true]
-      end
-
     # TODO: need connection information to complete the required attributes
     # net.peer.name or net.peer.ip and net.peer.port
-    base_attributes =
-      Keyword.merge(result,
-        "db.type": db_type,
-        "db.statement": query,
-        source: source,
-        "db.instance": database,
-        "db.url": url,
-        "total_time_#{time_unit}s": System.convert_time_unit(total_time, :native, time_unit)
-      )
+    base_attributes = [
+      "db.type": db_type,
+      "db.statement": query,
+      source: source,
+      "db.instance": database,
+      "db.url": url,
+      "total_time_#{time_unit}s": System.convert_time_unit(total_time, :native, time_unit)
+    ]
 
     attributes =
       measurements
@@ -101,6 +94,20 @@ defmodule OpentelemetryEcto do
         kind: :client
       })
 
+    case query_result do
+      {:error, error} ->
+        OpenTelemetry.Span.set_status(s, OpenTelemetry.status(:error, format_error(error)))
+
+      {:ok, _} ->
+        :ok
+    end
+
     OpenTelemetry.Span.end_span(s)
   end
+
+  defp format_error(%{__exception__: true} = exception) do
+    Exception.message(exception)
+  end
+
+  defp format_error(_), do: ""
 end
diff --git a/instrumentation/opentelemetry_ecto/test/opentelemetry_ecto_test.exs b/instrumentation/opentelemetry_ecto/test/opentelemetry_ecto_test.exs
index 8898ca1..4925396 100644
--- a/instrumentation/opentelemetry_ecto/test/opentelemetry_ecto_test.exs
+++ b/instrumentation/opentelemetry_ecto/test/opentelemetry_ecto_test.exs
@@ -1,8 +1,10 @@
 defmodule OpentelemetryEctoTest do
+  use ExUnit.Case
+  import Ecto.Query
+  require OpenTelemetry.Tracer
+
   alias OpentelemetryEcto.TestRepo, as: Repo
   alias OpentelemetryEcto.TestModels.{User, Post}
-  require OpenTelemetry.Tracer
-  use ExUnit.Case
 
   @event_name [:opentelemetry_ecto, :test_repo]
 
@@ -102,6 +104,24 @@ defmodule OpentelemetryEctoTest do
     assert_receive {:span, span(name: "opentelemetry_ecto.test_repo.query:posts")}
   end
 
+  test "sets error message on error" do
+    attach_handler()
+
+    try do
+      Repo.all(from u in "users", select: u.non_existant_field)
+    rescue
+      _ -> :ok
+    end
+
+    assert_receive {:span,
+                    span(
+                      name: "opentelemetry_ecto.test_repo.query:users",
+                      status: {:status, :error, message}
+                    )}
+
+    assert message =~ "non_existant_field does not exist"
+  end
+
   def attach_handler(config \\ []) do
     # For now setup the handler manually in each test
     handler = {__MODULE__, self()}